Skip to content

Commit afd7797

Browse files
arobenaroben
committed
Teach CoreIPC how to handle messages that are larger than the pipe's buffer
::GetOverlappedResult and ::ReadFile can fail with ERROR_MORE_DATA when there is more data available on the pipe than was requested in the read operation. In those cases, the appropriate response is to perform another read operation to read the extra data. We now do this. Also, MSDN says that, because we are doing asynchronous read operations, we should not pass a pointer to ::ReadFile to find out how many bytes were read. Instead we should always call ::GetOverlappedResult to find this out. I've changed Connection::readEventHandler to have a single loop that calls ::GetOverlappedResult and ::ReadFile in alternation, rather than sometimes calling ::ReadFile multiple times in a row, to satisfy this requirement. In order to simplify the logic in this function, I've made us request only a single byte from the pipe when there are no messages already in the pipe. (Previously we were requesting 4096 bytes in this case.) This allows us not to have to consider the case where the received message is smaller than our read buffer. If we decide that this has a negative impact on performance, we can of course change it. I've mitigated this somewhat by using ::PeekNamedMessage to find out the size of the next message in the pipe (if any), so that we can read it all in one read operation. Fixes <http://webkit.org/b/42710> <rdar://problem/8197571> Assertion in Connection::readEventHandler when launching WebKitTestRunner Reviewed by Anders Carlsson. * Platform/CoreIPC/win/ConnectionWin.cpp: (CoreIPC::Connection::readEventHandler): Put the call to ::GetOverlappedResult in the same loop as ::ReadFile so that we will call them alternately. If ::GetOverlappedResult fails with ERROR_MORE_DATA, use ::PeekNamedPipe to determine the size of the rest of the message, then read it from the pipe. After dispatching the message, use ::PeekNamedPipe to find out the size of the next message in the pipe so we can read it all in one operation. If there's no message in the pipe, we'll request just a single byte of the next message that becomes available, and Windows will tell us when the rest of the message is ready. If ::ReadFile fails with ERROR_MORE_DATA it means there is data available now even though we didn't think there was any. We go back to the top of the loop in this case and call ::GetOverlappedResult again to retrieve the available data. Canonical link: https://commits.webkit.org/54689@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@63852 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent c0f674c commit afd7797

2 files changed

Lines changed: 122 additions & 19 deletions

File tree

‎WebKit2/ChangeLog‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,52 @@
1+
2010-07-21 Adam Roben <aroben@apple.com>
2+
3+
Teach CoreIPC how to handle messages that are larger than the pipe's
4+
buffer
5+
6+
::GetOverlappedResult and ::ReadFile can fail with ERROR_MORE_DATA
7+
when there is more data available on the pipe than was requested in
8+
the read operation. In those cases, the appropriate response is to
9+
perform another read operation to read the extra data. We now do this.
10+
11+
Also, MSDN says that, because we are doing asynchronous read
12+
operations, we should not pass a pointer to ::ReadFile to find out how
13+
many bytes were read. Instead we should always call
14+
::GetOverlappedResult to find this out. I've changed
15+
Connection::readEventHandler to have a single loop that calls
16+
::GetOverlappedResult and ::ReadFile in alternation, rather than
17+
sometimes calling ::ReadFile multiple times in a row, to satisfy this
18+
requirement.
19+
20+
In order to simplify the logic in this function, I've made us request
21+
only a single byte from the pipe when there are no messages already in
22+
the pipe. (Previously we were requesting 4096 bytes in this case.)
23+
This allows us not to have to consider the case where the received
24+
message is smaller than our read buffer. If we decide that this has a
25+
negative impact on performance, we can of course change it. I've
26+
mitigated this somewhat by using ::PeekNamedMessage to find out the
27+
size of the next message in the pipe (if any), so that we can read it
28+
all in one read operation.
29+
30+
Fixes <http://webkit.org/b/42710> <rdar://problem/8197571> Assertion
31+
in Connection::readEventHandler when launching WebKitTestRunner
32+
33+
Reviewed by Anders Carlsson.
34+
35+
* Platform/CoreIPC/win/ConnectionWin.cpp:
36+
(CoreIPC::Connection::readEventHandler): Put the call to
37+
::GetOverlappedResult in the same loop as ::ReadFile so that we will
38+
call them alternately. If ::GetOverlappedResult fails with
39+
ERROR_MORE_DATA, use ::PeekNamedPipe to determine the size of the rest
40+
of the message, then read it from the pipe. After dispatching the
41+
message, use ::PeekNamedPipe to find out the size of the next message
42+
in the pipe so we can read it all in one operation. If there's no
43+
message in the pipe, we'll request just a single byte of the next
44+
message that becomes available, and Windows will tell us when the rest
45+
of the message is ready. If ::ReadFile fails with ERROR_MORE_DATA it
46+
means there is data available now even though we didn't think there
47+
was any. We go back to the top of the loop in this case and call
48+
::GetOverlappedResult again to retrieve the available data.
49+
150
2010-07-21 Sam Weinig <sam@webkit.org>
251

352
Reviewed by Anders Carlsson.

‎WebKit2/Platform/CoreIPC/win/ConnectionWin.cpp‎

Lines changed: 73 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -108,16 +108,40 @@ void Connection::readEventHandler()
108108
sendOutgoingMessages();
109109
}
110110

111-
DWORD numberOfBytesRead = 0;
112-
if (wasConnected) {
111+
while (true) {
113112
// Check if we got some data.
113+
DWORD numberOfBytesRead = 0;
114114
if (!::GetOverlappedResult(m_connectionPipe, &m_readState, &numberOfBytesRead, FALSE)) {
115115
DWORD error = ::GetLastError();
116116

117117
switch (error) {
118118
case ERROR_BROKEN_PIPE:
119119
connectionDidClose();
120120
return;
121+
case ERROR_MORE_DATA: {
122+
// Read the rest of the message out of the pipe.
123+
124+
DWORD bytesToRead = 0;
125+
if (!::PeekNamedPipe(m_connectionPipe, 0, 0, 0, 0, &bytesToRead)) {
126+
DWORD error = ::GetLastError();
127+
ASSERT_NOT_REACHED();
128+
return;
129+
}
130+
131+
// ::GetOverlappedResult told us there's more data. ::PeekNamedPipe shouldn't
132+
// contradict it!
133+
ASSERT(bytesToRead);
134+
if (!bytesToRead)
135+
break;
136+
137+
m_readBuffer.grow(m_readBuffer.size() + bytesToRead);
138+
if (!::ReadFile(m_connectionPipe, m_readBuffer.data() + numberOfBytesRead, bytesToRead, 0, &m_readState)) {
139+
DWORD error = ::GetLastError();
140+
ASSERT_NOT_REACHED();
141+
return;
142+
}
143+
continue;
144+
}
121145

122146
// FIXME: We should figure out why we're getting this error.
123147
case ERROR_IO_INCOMPLETE:
@@ -126,39 +150,69 @@ void Connection::readEventHandler()
126150
ASSERT_NOT_REACHED();
127151
}
128152
}
129-
}
130153

131-
while (true) {
132-
if (numberOfBytesRead) {
154+
if (!m_readBuffer.isEmpty()) {
133155
// We have a message, let's dispatch it.
134156

135157
// The messageID is encoded at the end of the buffer.
136-
size_t realBufferSize = numberOfBytesRead - sizeof(MessageID);
158+
// Note that we assume here that the message is the same size as m_readBuffer. We can
159+
// assume this because we always size m_readBuffer to exactly match the size of the message,
160+
// either when receiving ERROR_MORE_DATA from ::GetOverlappedResult above or when
161+
// ::PeekNamedPipe tells us the size below. We never set m_readBuffer to a size larger
162+
// than the message.
163+
size_t realBufferSize = m_readBuffer.size() - sizeof(MessageID);
137164

138165
unsigned messageID = *reinterpret_cast<unsigned*>(m_readBuffer.data() + realBufferSize);
139166

140167
processIncomingMessage(MessageID::fromInt(messageID), adoptPtr(new ArgumentDecoder(m_readBuffer.data(), realBufferSize)));
141168
}
142169

143-
// FIXME: Do this somewhere else.
144-
m_readBuffer.resize(inlineMessageMaxSize);
170+
// Find out the size of the next message in the pipe (if there is one) so that we can read
171+
// it all in one operation. (This is just an optimization to avoid an extra pass through the
172+
// loop (if we chose a buffer size that was too small) or allocating extra memory (if we
173+
// chose a buffer size that was too large).)
174+
DWORD bytesToRead = 0;
175+
if (!::PeekNamedPipe(m_connectionPipe, 0, 0, 0, 0, &bytesToRead)) {
176+
DWORD error = ::GetLastError();
177+
ASSERT_NOT_REACHED();
178+
}
179+
if (!bytesToRead) {
180+
// There's no message waiting in the pipe. Schedule a read of the first byte of the
181+
// next message. We'll find out the message's actual size when it arrives. (If we
182+
// change this to read more than a single byte for performance reasons, we'll have to
183+
// deal with m_readBuffer potentially being larger than the message we read after
184+
// calling ::GetOverlappedResult above.)
185+
bytesToRead = 1;
186+
}
145187

146-
// Read from the pipe.
147-
BOOL result = ::ReadFile(m_connectionPipe, m_readBuffer.data(), m_readBuffer.size(), &numberOfBytesRead, &m_readState);
188+
m_readBuffer.resize(bytesToRead);
148189

149-
if (!result) {
150-
DWORD error = ::GetLastError();
190+
// Either read the next available message (which should occur synchronously), or start an
191+
// asynchronous read of the next message that becomes available.
192+
BOOL result = ::ReadFile(m_connectionPipe, m_readBuffer.data(), m_readBuffer.size(), 0, &m_readState);
193+
if (result) {
194+
// There was already a message waiting in the pipe, and we read it synchronously.
195+
// Process it.
196+
continue;
197+
}
151198

152-
if (error == ERROR_IO_PENDING) {
153-
// There's pending data - readEventHandler will be called again once the read is complete.
154-
return;
155-
}
199+
DWORD error = ::GetLastError();
156200

157-
// FIXME: We need to handle other errors here.
158-
ASSERT_NOT_REACHED();
201+
if (error == ERROR_IO_PENDING) {
202+
// There are no messages in the pipe currently. readEventHandler will be called again once there is a message.
203+
return;
204+
}
205+
206+
if (error == ERROR_MORE_DATA) {
207+
// Either a message is available when we didn't think one was, or the message is larger
208+
// than ::PeekNamedPipe told us. The former seems far more likely. Probably the message
209+
// became available between our calls to ::PeekNamedPipe and ::ReadFile above. Go back
210+
// to the top of the loop to use ::GetOverlappedResult to retrieve the available data.
211+
continue;
159212
}
160213

161-
ASSERT(numberOfBytesRead);
214+
// FIXME: We need to handle other errors here.
215+
ASSERT_NOT_REACHED();
162216
}
163217
}
164218

0 commit comments

Comments
 (0)