PDFPlugin hangs in deallocation when data load contends with waiting for thread completion #25448

cdumez · 2024-03-04T21:19:20Z

`e604d7c`

PDFPlugin hangs in deallocation when data load contends with waiting for thread completion
https://bugs.webkit.org/show_bug.cgi?id=246454
rdar://101147811

Reviewed by Tim Horton.

When `PDFPlugin::teardown()` would get called (on the main thread), it would
wait for the PDF thread to complete. However, if loading is still going on,
the PDF thread might be blocked on `dataSemaphore.wait()` in
PDFIncrementalLoader::dataProviderGetBytesAtPosition(). The `dataSemaphore`
is supposed to get signaled on a task dispatched to the main thread but this
task won't run if the main thread is blocked on waiting for the thread to
exit.

To address the issue, PDFIncrementalLoader::clear() now signals the
pending dataSemaphores before waiting for the thread to exit and sets a
flag indicating we want the thread to exit. This flag is then checked
on the PDF thread to make sure we don't queue additional work / semaphores.

* LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction-expected.txt: Added.
* LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction.html: Added.
I took the test from Erica Li's earlier PR that got reverted.

* Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h:
(WebKit::PDFIncrementalLoader::SemaphoreWrapper::create):
(WebKit::PDFIncrementalLoader::SemaphoreWrapper::wait):
(WebKit::PDFIncrementalLoader::SemaphoreWrapper::signal):
(WebKit::PDFIncrementalLoader::WTF_GUARDED_BY_LOCK):
* Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm:
(WebKit::PDFIncrementalLoader::clear):
(WebKit::PDFIncrementalLoader::dataProviderGetBytesAtPosition):
(WebKit::PDFIncrementalLoader::dataProviderGetByteRanges):
(WebKit::PDFIncrementalLoader::threadEntry):

Canonical link: https://commits.webkit.org/275707@main

5ffcd92

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-03-04T21:19:27Z

EWS run on previous version of this PR (hash 93c2661)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	❌ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	❌ 🧪 api-mac	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 gtk
	~~🧪 api-ios~~	❌ 🧪 mac-wk2	⏳ 🧪 gtk-wk2
	~~🛠 tv~~	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 api-gtk
	~~🛠 tv-sim~~	❌ 🧪 mac-wk2-stress
	~~🛠 watch~~
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-03-04T22:09:04Z

EWS run on previous version of this PR (hash 6ac3495)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-03-05T02:52:28Z

EWS run on previous version of this PR (hash f9f5332)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	⏳ 🧪 api-mac	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	⏳ 🧪 api-gtk
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress
	~~🛠 watch~~
	✅ 🛠 watch-sim

Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h

Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm

lericaa · 2024-03-05T04:40:50Z

Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm

        for (size_t i = 0; i < count; ++i) {
-            protectedLoader->getResourceBytesAtPosition(ranges[i].length, ranges[i].location, [i, &dataResults, &dataSemaphore](const uint8_t* bytes, size_t bytesCount) {
+            protectedLoader->getResourceBytesAtPosition(ranges[i].length, ranges[i].location, [i, &dataResults, callbackAggregator](const uint8_t* bytes, size_t bytesCount) {


🤔 not sure how callbackAggregator works but will it get called for each loop? if that is the case, we just wait one time, and it is a BinarySemaphore. (i was quite hesitate if we should also address for this callback)

callbackAggregator calls its lambda on destruction. Since I capture it by value in each lambda inside the loop, we will only signal the semaphore ONCE, when all getResourceBytesAtPosition() calls have completed. As a result, we only need a single semaphore / a single wait(). This simplifies the code.

webkit-early-warning-system · 2024-03-05T05:52:14Z

EWS run on previous version of this PR (hash 506a859)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
~~🧪 style~~	~~🛠 ios~~	~~🛠 mac~~	~~🛠 wpe~~	~~🛠 wincairo~~
	~~🛠 ios-sim~~	~~🛠 mac-AS-debug~~	~~🧪 wpe-wk2~~
~~🧪 webkitperl~~	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🛠 gtk~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
	~~🛠 tv~~	~~🧪 mac-AS-debug-wk2~~	~~🧪 api-gtk~~
	~~🛠 tv-sim~~	~~🧪 mac-wk2-stress~~
	~~🛠 watch~~
	~~🛠 watch-sim~~

webkit-early-warning-system · 2024-03-05T05:52:15Z

EWS run on current version of this PR (hash 5ffcd92)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

…for thread completion https://bugs.webkit.org/show_bug.cgi?id=246454 rdar://101147811 Reviewed by Tim Horton. When `PDFPlugin::teardown()` would get called (on the main thread), it would wait for the PDF thread to complete. However, if loading is still going on, the PDF thread might be blocked on `dataSemaphore.wait()` in PDFIncrementalLoader::dataProviderGetBytesAtPosition(). The `dataSemaphore` is supposed to get signaled on a task dispatched to the main thread but this task won't run if the main thread is blocked on waiting for the thread to exit. To address the issue, PDFIncrementalLoader::clear() now signals the pending dataSemaphores before waiting for the thread to exit and sets a flag indicating we want the thread to exit. This flag is then checked on the PDF thread to make sure we don't queue additional work / semaphores. * LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction-expected.txt: Added. * LayoutTests/compositing/plugins/pdf/pdf-plugin-hang-during-destruction.html: Added. I took the test from Erica Li's earlier PR that got reverted. * Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h: (WebKit::PDFIncrementalLoader::SemaphoreWrapper::create): (WebKit::PDFIncrementalLoader::SemaphoreWrapper::wait): (WebKit::PDFIncrementalLoader::SemaphoreWrapper::signal): (WebKit::PDFIncrementalLoader::WTF_GUARDED_BY_LOCK): * Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm: (WebKit::PDFIncrementalLoader::clear): (WebKit::PDFIncrementalLoader::dataProviderGetBytesAtPosition): (WebKit::PDFIncrementalLoader::dataProviderGetByteRanges): (WebKit::PDFIncrementalLoader::threadEntry): Canonical link: https://commits.webkit.org/275707@main

webkit-commit-queue · 2024-03-05T20:52:19Z

Committed 275707@main (e604d7c): https://commits.webkit.org/275707@main

Reviewed commits have been landed. Closing PR #25448 and removing active labels.

cdumez self-assigned this Mar 4, 2024

cdumez added the PDF For bugs in WebKit's built-in PDF support. label Mar 4, 2024

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 4, 2024

cdumez removed the merging-blocked Applied to prevent a change from being merged label Mar 4, 2024

cdumez force-pushed the 246454_PDFPlugin_hang branch from 93c2661 to 6ac3495 Compare March 4, 2024 22:08

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 5, 2024

cdumez removed the merging-blocked Applied to prevent a change from being merged label Mar 5, 2024

cdumez force-pushed the 246454_PDFPlugin_hang branch from 6ac3495 to f9f5332 Compare March 5, 2024 02:52

cdumez marked this pull request as ready for review March 5, 2024 04:17

cdumez requested review from smfr, hortont424 and beidson March 5, 2024 04:17

lericaa reviewed Mar 5, 2024

View reviewed changes

Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h Show resolved Hide resolved

lericaa reviewed Mar 5, 2024

View reviewed changes

Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm Show resolved Hide resolved

lericaa reviewed Mar 5, 2024

View reviewed changes

cdumez force-pushed the 246454_PDFPlugin_hang branch 2 times, most recently from 506a859 to 5ffcd92 Compare March 5, 2024 05:44

hortont424 approved these changes Mar 5, 2024

View reviewed changes

cdumez added the merge-queue Applied to send a pull request to merge-queue label Mar 5, 2024

webkit-commit-queue force-pushed the 246454_PDFPlugin_hang branch from 5ffcd92 to e604d7c Compare March 5, 2024 20:52

webkit-commit-queue merged commit e604d7c into WebKit:main Mar 5, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PDFPlugin hangs in deallocation when data load contends with waiting for thread completion #25448

PDFPlugin hangs in deallocation when data load contends with waiting for thread completion #25448

Uh oh!

cdumez commented Mar 4, 2024 •

edited by webkit-early-warning-system

Loading

Uh oh!

webkit-early-warning-system commented Mar 4, 2024 •

edited

Loading

Uh oh!

webkit-early-warning-system commented Mar 4, 2024 •

edited

Loading

Uh oh!

webkit-early-warning-system commented Mar 5, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

lericaa Mar 5, 2024 •

edited

Loading

Uh oh!

cdumez Mar 5, 2024 •

edited

Loading

Uh oh!

webkit-early-warning-system commented Mar 5, 2024 •

edited

Loading

Uh oh!

webkit-early-warning-system commented Mar 5, 2024 •

edited

Loading

Uh oh!

webkit-commit-queue commented Mar 5, 2024

Uh oh!

Uh oh!

PDFPlugin hangs in deallocation when data load contends with waiting for thread completion #25448

PDFPlugin hangs in deallocation when data load contends with waiting for thread completion #25448

Uh oh!

Conversation

cdumez commented Mar 4, 2024 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

e604d7c

Uh oh!

webkit-early-warning-system commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lericaa Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdumez Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Mar 5, 2024

Uh oh!

Uh oh!

cdumez commented Mar 4, 2024 •

edited by webkit-early-warning-system

Loading

`e604d7c`

webkit-early-warning-system commented Mar 4, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 4, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 5, 2024 •

edited

Loading

lericaa Mar 5, 2024 •

edited

Loading

cdumez Mar 5, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 5, 2024 •

edited

Loading

webkit-early-warning-system commented Mar 5, 2024 •

edited

Loading