Bug 222546 – WebGL asserts after GPU process times out

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kimmo Kinnunen]

WebGL asserts after GPU process times out

[Kimmo Kinnunen]

Created attachment 421912 [details]
Patch

[Kimmo Kinnunen]

Created attachment 421927 [details]
Patch

[Kimmo Kinnunen]

Created attachment 422229 [details]
Patch

[Kimmo Kinnunen]

Created attachment 422267 [details]
Patch

[Kimmo Kinnunen]

Created attachment 422269 [details]
Patch

[Wenson Hsieh]

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`).

[Kimmo Kinnunen]

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?

[EWS]

Committed r274066: <https://commits.webkit.org/r274066>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422269 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75163452>