Bug 222546

Created attachment 421912 [details] Patch

Created attachment 421927 [details] Patch

Created attachment 422229 [details] Patch

Created attachment 422267 [details] Patch

Created attachment 422269 [details] Patch

Comment on attachment 422269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422269&action=review > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:-364 > - bool found = m_remoteGraphicsContextGLMap.remove(graphicsContextGLIdentifier); > - ASSERT_UNUSED(found, found); Not new to this patch, but I wonder if we ought to `MESSAGE_CHECK` the incoming `graphicsContextGLIdentifier`. (The same comment seems to apply to `RemoteRenderingBackend` as well). > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:369 > + releaseGraphicsContextGL(identifier); (Ditto) > Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:265 > + if (auto connectionToWeb = gpuConnectionToWebProcess.get()) `if (auto connectionToWeb = makeRefPtr(gpuConnectionToWebProcess.get()))`? > Source/WebKit/Platform/IPC/StreamClientConnection.h:174 > + // Since the value is trusted, we only assert. > + ASSERT(clientLimit != ClientLimit::clientIsWaitingTag); This seems like a good opportunity for a MESSAGE_CHECK as well. > Source/WebKit/Platform/IPC/StreamServerConnection.cpp:129 > + return std::min(limit, dataSize() - 1); It seems like this could be made more robust to underflow (i.e. the `dataSize() - 1`).

Comment on attachment 422269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=422269&action=review Thanks for spending the time! >> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:-364 >> - ASSERT_UNUSED(found, found); > > Not new to this patch, but I wonder if we ought to `MESSAGE_CHECK` the incoming `graphicsContextGLIdentifier`. (The same comment seems to apply to `RemoteRenderingBackend` as well). Not really. (In the current design) There are cases where the sender of ReleaseGraphicsContextGL cannot know if the receiver exists. Also, the sender does not care. It is a release operation. From the receiver perspective, the receiver treats all unknown identifiers as such: the underlying objects were already discarded. So if we send: CreateGraphicsContextGL 1 DrawSomething 1 ReleaseGraphicsContextGL 1 We know DrawSomething can cause context lost. In the current implementation of losing context, we discard the context immediately when context lost happens. In the current implementation of the communication, by definition, if we receive a message to unknown object, it is a message to already discarded context. It these scenarios unknown object is a valid thing, and should not assert, cause errors or anything out of ordinary. >> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:369 >> + releaseGraphicsContextGL(identifier); > > (Ditto) Same as above. So this one is called when we want to test timeout. If we send: CreateGraphicsContextGL 1 DrawSomething 1 SimulateEventForTesting 1, Timeout ReleaseGraphicsContextGL 1 So we in general cannot assume that "DrawSomething" did not cause context lost. In current implementation of losing context,: If the command DrawSomething caused context lost, SimulateEventForTesting 1, Timeout is still a valid message -- it is just discarded. It is not a message that should assert, cause error printout or stand out in any way. >> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:265 >> + if (auto connectionToWeb = gpuConnectionToWebProcess.get()) > > `if (auto connectionToWeb = makeRefPtr(gpuConnectionToWebProcess.get()))`? No, RemoteGraphicsContextGL does not take an owning reference to gpuConnectionToWebProcess. Maybe expand what did you have in mind? >> Source/WebKit/Platform/IPC/StreamClientConnection.h:174 >> + ASSERT(clientLimit != ClientLimit::clientIsWaitingTag); > > This seems like a good opportunity for a MESSAGE_CHECK as well. No. If I understand correctly, "MESSAGE_CHECK" is a macro to check some property of IPC protocol in the shared data. This is just a normal assertion asserting: Caller called tryAcquire The call timed out Caller called tryAcquire again, even though this is not defined since the previous call timed out. Even though it loads the shared variable, the variable is the variable *this part of the code* modifies. Since here the other part is trusted and since the other part never writes this value asserted here, it is not something MESSAGE_CHECK should check -- it's not a communication protocol error. So this is like saying void myFunc() { ASSERT(!m_didTimeout); m_didTimeout = true; } >> Source/WebKit/Platform/IPC/StreamServerConnection.cpp:129 >> + return std::min(limit, dataSize() - 1); > > It seems like this could be made more robust to underflow (i.e. the `dataSize() - 1`). Did not get this one, could you expand?

Committed r274066: <https://commits.webkit.org/r274066> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422269 [details].

<rdar://problem/75163452>