Bug 221920 – [GPUP] Ignore an IPC message if the message receiver has been destroyed

• Component: New Bugs
• Status: RESOLVED FIXED

[Peng Liu]

[GPUP] Ignore an IPC message if the message receiver has been destroyed

[Peng Liu]

Created attachment 420358 [details]
Patch

[Eric Carlson]

Comment on attachment 420358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420358&action=review

> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:110
> +Logger& GPUProcessConnection::logger()
> +{
> +    if (!m_logger)
> +        m_logger = Logger::create(this);
> +
> +    return *m_logger;
> +}
> +

It looks like this is unused, `RELEASE_LOG_ERROR` does not use a Logger.

[Peng Liu]

Comment on attachment 420358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420358&action=review

>> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:110
>> +
> 
> It looks like this is unused, `RELEASE_LOG_ERROR` does not use a Logger.

Right, will fix it. Thanks!

[Peng Liu]

Created attachment 420374 [details]
Revise the patch based on Eric's comment

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/74369181>

[Truitt Savell]

We are still seeing these two tests crashing even after this change:
imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-origin.sub.html
http/tests/navigation/parsed-iframe-dynamic-form-back-entry.html

history:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=http%2Ftests%2Fnavigation%2Fparsed-iframe-dynamic-form-back-entry.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fcanvas%2Felement%2Fimagebitmap%2FcreateImageBitmap-origin.sub.html

[Truitt Savell]

Crash log for imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-origin.sub.html

https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/r272998%20(200)/imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-origin.sub-crash-log.txt


Crash log for http/tests/navigation/parsed-iframe-dynamic-form-back-entry.html

https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/r272998%20(200)/http/tests/navigation/parsed-iframe-dynamic-form-back-entry-crash-log.txt

[Truitt Savell]

Reopening for continuing issues

[Peng Liu]

(In reply to Truitt Savell from comment #8)
> Crash log for
> imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/
> createImageBitmap-origin.sub.html
> 
> https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/
> r272998%20(200)/imported/w3c/web-platform-tests/html/canvas/element/
> imagebitmap/createImageBitmap-origin.sub-crash-log.txt
> 
> 
> Crash log for
> http/tests/navigation/parsed-iframe-dynamic-form-back-entry.html
> 
> https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/
> r272998%20(200)/http/tests/navigation/parsed-iframe-dynamic-form-back-entry-
> crash-log.txt

These are GPU Process crashes.

[Peng Liu]

Filed bug 222079 to fix the crash of http/tests/navigation/parsed-iframe-dynamic-form-back-entry.html.

[Peng Liu]

Bug 221989 tracks the crash of imported/w3c/web-platform-tests/html/canvas/element/imagebitmap/createImageBitmap-origin.sub.html.

[Peng Liu]

Let's close this bug.

[Kimmo Kinnunen]

Comment on attachment 420374 [details]
Revise the patch based on Eric's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=420374&action=review

> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:181
> +        RELEASE_LOG_ERROR(WebGL, "The RemoteGraphicsContextGLProxy object has beed destroyed");

From WebGL perspective this is incorrect.
This is not an error. This is expected, the sender cannot know when the receiver decides to stop listening.

[Peng Liu]

Comment on attachment 420374 [details]
Revise the patch based on Eric's comment

View in context: https://bugs.webkit.org/attachment.cgi?id=420374&action=review

>> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:181
>> +        RELEASE_LOG_ERROR(WebGL, "The RemoteGraphicsContextGLProxy object has beed destroyed");
> 
> From WebGL perspective this is incorrect.
> This is not an error. This is expected, the sender cannot know when the receiver decides to stop listening.

Hmm, but this log message is from GPUProcessConnection's perspective?

[Jer Noble]

(In reply to Kimmo Kinnunen from comment #14)
> Comment on attachment 420374 [details]
> Revise the patch based on Eric's comment
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420374&action=review
> 
> > Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:181
> > +        RELEASE_LOG_ERROR(WebGL, "The RemoteGraphicsContextGLProxy object has beed destroyed");
> 
> From WebGL perspective this is incorrect.
> This is not an error. This is expected, the sender cannot know when the
> receiver decides to stop listening.

This is immaterial to whether the receiver is a WebGL object or not; this is a bug in our XPC implementation.  And it's not true that the sender cannot know when the receiver decides to stop listening. The receiver can send an async message to the remote process and wait for a response before stopping listening.

[Kimmo Kinnunen]

(In reply to Jer Noble from comment #16)
> And it's not true that the sender cannot
> know when the receiver decides to stop listening. The receiver can send an
> async message to the remote process and wait for a response before stopping
> listening.

Web process sends "do something"  for destination id 7.
GPU process receives "do something".
GPU process sends "something done".
Web process sends "do something" else for destination id 7.
Web process notices GPU process timed out.
Web process removes all references to anything related to id 7.
Web process receives "something done" for id 7.

In general, in asynchronous communication you cannot know if the other party will get the message you send.

[Kimmo Kinnunen]

> The receiver can send an
> async message to the remote process and wait for a response before stopping
> listening.

The destruction is not a sync operation. Would it be useful as as sync operation?

Here's a sequence that might trigger the error message

WebContent:
Send CreateRemoteGraphicsContextGL id 7
Send ReleaseRemoteGraphicsContextGL id 7

GPU Process:
Got CreateRemoteGraphicsContextGL id 7
Send WasCreated id 7
Got ReleaseRemoteGraphicsContextGL id 7

[Kimmo Kinnunen]

(In reply to Kimmo Kinnunen from comment #18)
> The destruction is not a sync operation. Would it be useful as as sync
> operation?

Sync or async with a reply would fit here better -- does not matter. 

Destruction is not usually something you compute a result with.