Bug 232113 – REGRESSION (r284079): Audio continues playing on hulu.com in private browsing mode after closing the tab

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

rdar://84399283

[Wenson Hsieh]

Created attachment 442099 [details]
For EWS

[Wenson Hsieh]

Created attachment 442100 [details]
Minor renaming

[Wenson Hsieh]

*** Bug 232139 has been marked as a duplicate of this bug. ***

[Wenson Hsieh]

Comment on attachment 442100 [details]
Minor renaming

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

> Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.h:152
>      Ref<WebCore::ImageBuffer> m_imageBuffer;

This backpointer should be weak, since RemoteImageBuffer (strongly) owns its RemoteDisplayListRecorder.

Going to make this adjustment along with the rest of this patch.

[Wenson Hsieh]

Created attachment 442184 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 442189 [details]
Add missing 'Reviewed by' line

[Wenson Hsieh]

Created attachment 442191 [details]
Add missing 'Reviewed by' line

[Wenson Hsieh]

Created attachment 442206 [details]
For EWS

[Chris Dumez]

Comment on attachment 442206 [details]
For EWS

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

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:173
> +    inline static uint64_t objectCountForTesting() { return gObjectCountForTesting; }

inline seems redundant?

> Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp:424
> +    m_renderingBackend->performWithMediaPlayerOnMainThread(identifier, [RefPtr imageBuffer = m_imageBuffer.get(), destination](MediaPlayer& player) {

I think we usually use `imageBuffer = RefPtr { m_imageBuffer.get() }` ?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:137
> +    Lock m_remoteDisplayListsLock;

That seems a bit fishy given that m_remoteDisplayLists is a WeakHashSet. How could it be used safely from multiple threads given that WeakPtr cannot?

[Wenson Hsieh]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 442206 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442206&action=review
> 
> > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.h:173
> > +    inline static uint64_t objectCountForTesting() { return gObjectCountForTesting; }
> 
> inline seems redundant?

Good point — removed the `inline`.

> 
> > Source/WebKit/GPUProcess/graphics/RemoteDisplayListRecorder.cpp:424
> > +    m_renderingBackend->performWithMediaPlayerOnMainThread(identifier, [RefPtr imageBuffer = m_imageBuffer.get(), destination](MediaPlayer& player) {
> 
> I think we usually use `imageBuffer = RefPtr { m_imageBuffer.get() }` ?

👍🏻

> 
> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.h:137
> > +    Lock m_remoteDisplayListsLock;
> 
> That seems a bit fishy given that m_remoteDisplayLists is a WeakHashSet. How
> could it be used safely from multiple threads given that WeakPtr cannot?

That's true — though, I'm curious why this isn't already causing all sorts of "!= constructed on main thread" assertions already, given that this is already an issue.

I'm going to clean this up in https://bugs.webkit.org/show_bug.cgi?id=232183, and leave it as it is currently. I plan to turn this into a regular HashSet of either RefPtr or Ref, and manually remove entries when image buffers are released (based on rendering resource ID)

[Wenson Hsieh]

Created attachment 442207 [details]
For EWS

[Wenson Hsieh]

Created attachment 442321 [details]
Rebase on trunk

[Wenson Hsieh]

Created attachment 442330 [details]
Try to fix layout test?

[Chris Dumez]

Comment on attachment 442330 [details]
Try to fix layout test?

r=me

[EWS]

Committed r284786 (243495@main): <https://commits.webkit.org/243495@main>

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