Bug 232113

Summary: REGRESSION (r284079): Audio continues playing on hulu.com in private browsing mode after closing the tab
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit2Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, kkinnunen, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 232179    
Bug Blocks:    
Attachments:
Description Flags
For EWS
none
Minor renaming
none
Rebase on trunk
none
Add missing 'Reviewed by' line
none
Add missing 'Reviewed by' line
none
For EWS
ews-feeder: commit-queue-
For EWS
none
Rebase on trunk
none
Try to fix layout test? none

Wenson Hsieh
Reported 2021-10-21 16:06:05 PDT
rdar://84399283
Attachments
For EWS (25.86 KB, patch)
2021-10-21 18:11 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Minor renaming (25.90 KB, patch)
2021-10-21 18:15 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Rebase on trunk (27.17 KB, patch)
2021-10-22 12:12 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Add missing 'Reviewed by' line (28.32 KB, patch)
2021-10-22 13:21 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Add missing 'Reviewed by' line (27.16 KB, patch)
2021-10-22 13:22 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
For EWS (26.92 KB, patch)
2021-10-22 15:40 PDT, Wenson Hsieh 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
For EWS (26.92 KB, patch)
2021-10-22 15:56 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Rebase on trunk (23.93 KB, patch)
2021-10-24 14:00 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Try to fix layout test? (26.38 KB, patch)
2021-10-24 16:47 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2021-10-21 18:11:28 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 2 2021-10-21 18:15:28 PDT
Created attachment 442100 [details] Minor renaming
Wenson Hsieh
Comment 3 2021-10-22 10:52:42 PDT
*** Bug 232139 has been marked as a duplicate of this bug. ***
Wenson Hsieh
Comment 4 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.
Wenson Hsieh
Comment 5 2021-10-22 12:12:13 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 6 2021-10-22 13:21:31 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 7 2021-10-22 13:22:45 PDT
Created attachment 442191 [details] Add missing 'Reviewed by' line
Wenson Hsieh
Comment 8 2021-10-22 15:40:42 PDT Comment hidden (obsolete)
Chris Dumez
Comment 9 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?
Wenson Hsieh
Comment 10 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)
Wenson Hsieh
Comment 11 2021-10-22 15:56:53 PDT
Created attachment 442207 [details] For EWS
Wenson Hsieh
Comment 12 2021-10-24 14:00:14 PDT
Created attachment 442321 [details] Rebase on trunk
Wenson Hsieh
Comment 13 2021-10-24 16:47:38 PDT
Created attachment 442330 [details] Try to fix layout test?
Chris Dumez
Comment 14 2021-10-25 08:06:22 PDT
Comment on attachment 442330 [details] Try to fix layout test? r=me
EWS
Comment 15 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].
Note You need to log in before you can comment on or make changes to this bug.