Bug 232113 - REGRESSION (r284079): Audio continues playing on hulu.com in private browsing mode after closing the tab
Summary: REGRESSION (r284079): Audio continues playing on hulu.com in private browsing...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
: 232139 (view as bug list)
Depends on: 232179
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-21 16:06 PDT by Wenson Hsieh
Modified: 2021-10-25 08:37 PDT (History)
4 users (show)

See Also:


Attachments
For EWS (25.86 KB, patch)
2021-10-21 18:11 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Minor renaming (25.90 KB, patch)
2021-10-21 18:15 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (27.17 KB, patch)
2021-10-22 12:12 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Add missing 'Reviewed by' line (28.32 KB, patch)
2021-10-22 13:21 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Add missing 'Reviewed by' line (27.16 KB, patch)
2021-10-22 13:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
For EWS (26.92 KB, patch)
2021-10-22 15:40 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
For EWS (26.92 KB, patch)
2021-10-22 15:56 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Rebase on trunk (23.93 KB, patch)
2021-10-24 14:00 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Try to fix layout test? (26.38 KB, patch)
2021-10-24 16:47 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2021-10-21 16:06:05 PDT
rdar://84399283
Comment 1 Wenson Hsieh 2021-10-21 18:11:28 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2021-10-21 18:15:28 PDT
Created attachment 442100 [details]
Minor renaming
Comment 3 Wenson Hsieh 2021-10-22 10:52:42 PDT
*** Bug 232139 has been marked as a duplicate of this bug. ***
Comment 4 Wenson Hsieh 2021-10-22 10:54:58 PDT
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.
Comment 5 Wenson Hsieh 2021-10-22 12:12:13 PDT Comment hidden (obsolete)
Comment 6 Wenson Hsieh 2021-10-22 13:21:31 PDT Comment hidden (obsolete)
Comment 7 Wenson Hsieh 2021-10-22 13:22:45 PDT
Created attachment 442191 [details]
Add missing 'Reviewed by' line
Comment 8 Wenson Hsieh 2021-10-22 15:40:42 PDT Comment hidden (obsolete)
Comment 9 Chris Dumez 2021-10-22 15:46:13 PDT
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?
Comment 10 Wenson Hsieh 2021-10-22 15:54:14 PDT
(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)
Comment 11 Wenson Hsieh 2021-10-22 15:56:53 PDT
Created attachment 442207 [details]
For EWS
Comment 12 Wenson Hsieh 2021-10-24 14:00:14 PDT
Created attachment 442321 [details]
Rebase on trunk
Comment 13 Wenson Hsieh 2021-10-24 16:47:38 PDT
Created attachment 442330 [details]
Try to fix layout test?
Comment 14 Chris Dumez 2021-10-25 08:06:22 PDT
Comment on attachment 442330 [details]
Try to fix layout test?

r=me
Comment 15 EWS 2021-10-25 08:37:25 PDT
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].