Bug 254374

Summary: REGRESSION (260451@main): Opening any PDF in WebKit opens it halfway down the first page
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: PDFAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Tyler Wilcock
Reported 2023-03-23 15:23:18 PDT
Before 260451@main, the sequence for updating PDF scroll position on and shortly after load was this: 1. PDF plugin is initialized, scroll position is set to (0, 0) 2. PDFKit does -[PDFLayerController _updateAutoScale] and -[PDFLayerController magnifyWithMagnification:atPoint:immediately:], and informs the WKPDFLayerControllerDelegate to update the scroll position to some greater than zero value (despite no scroll actually happening) 3. A call to PDFPlugin::receivedNonLinearizedPDFSentinel() is handled. Scroll position is set to (0, 0) After 260451@main, the sequence became: 1. PDF plugin is initialized, scroll position is set to (0, 0) 2. PDFKit does magnification as described above, updates scroll position to (0, 190 (or other >0 value)). This is dispatched to the main run loop to be handled asynchronously. 3. A call to PDFPlugin::receivedNonLinearizedPDFSentinel() is handled. Scroll position is set to (0, 0) 4. The async dispatch from step 2 is handled by the main-runloop, overwriting the (0,0) value rdar://106880773
Attachments
Patch (11.29 KB, patch)
2023-03-23 15:28 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Tyler Wilcock
Comment 1 2023-03-23 15:28:07 PDT
Created attachment 465558 [details] Patch
Radar WebKit Bug Importer
Comment 2 2023-03-23 15:35:56 PDT
<rdar://problem/107156793>
Tyler Wilcock
Comment 3 2023-03-23 15:36:09 PDT
rdar://106880773
Tim Horton
Comment 4 2023-03-23 15:55:01 PDT
Comment on attachment 465558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465558&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:455 > + callOnMainRunLoopAndWait([protectedPlugin = Ref { *_pdfPlugin }, newPosition] { What thread are we actually on, and are we sure the main thread will never be waiting on *us*?
Simon Fraser (smfr)
Comment 5 2023-03-23 16:37:33 PDT
Comment on attachment 465558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465558&action=review > LayoutTests/platform/mac-wk2/plugins/pdf-plugin-initial-scroll-position.html:1 > +<!DOCTYPE html> Could this be a ref test that tests against <img src=foo.pdf> rather than having to add all the testing interfaces?
Chris Dumez
Comment 6 2023-03-23 16:40:17 PDT
Comment on attachment 465558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=465558&action=review >> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:455 >> + callOnMainRunLoopAndWait([protectedPlugin = Ref { *_pdfPlugin }, newPosition] { > > What thread are we actually on, and are we sure the main thread will never be waiting on *us*? callOnMainRunLoopAndWait() is safe in WebKit. It will detect the case where we're already on the main run loop and run the lambda synchronously.
Tyler Wilcock
Comment 7 2023-03-23 17:59:37 PDT
(In reply to Tim Horton from comment #4) > Comment on attachment 465558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=465558&action=review > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:455 > > + callOnMainRunLoopAndWait([protectedPlugin = Ref { *_pdfPlugin }, newPosition] { > > What thread are we actually on, and are we sure the main thread will never > be waiting on *us*? If accessibility is enabled, it's possible for this method to be called on the secondary accessibility thread via -[PDFAccessibilityNodePage scrollToVisible] (e.g. when VoiceOver moves focus to an element in a different page). The main-thread only waits on the AX thread if it wants AXIsolatedTree::m_changeLogLock (to push AX tree updates), and the AX thread currently holds the lock (applying said updates). Neither -[WKPDFLayerControllerDelegate updateScrollPosition:] nor anything leading up to it causes the AX thread to acquire the lock.
Tyler Wilcock
Comment 8 2023-03-23 18:06:26 PDT
(In reply to Simon Fraser (smfr) from comment #5) > Comment on attachment 465558 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=465558&action=review > > > LayoutTests/platform/mac-wk2/plugins/pdf-plugin-initial-scroll-position.html:1 > > +<!DOCTYPE html> > > Could this be a ref test that tests against <img src=foo.pdf> rather than > having to add all the testing interfaces? Tried this out. When rendered in an embed, there are two gray borders framing the PDF content. There are also PDF controls in an overlay. Rendering the same PDF in an img produced neither of these, so I think a ref test may be tricky.
Tim Horton
Comment 9 2023-03-23 21:33:10 PDT
(In reply to Tyler Wilcock from comment #8) > (In reply to Simon Fraser (smfr) from comment #5) > > Comment on attachment 465558 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=465558&action=review > > > > > LayoutTests/platform/mac-wk2/plugins/pdf-plugin-initial-scroll-position.html:1 > > > +<!DOCTYPE html> > > > > Could this be a ref test that tests against <img src=foo.pdf> rather than > > having to add all the testing interfaces? > Tried this out. When rendered in an embed, there are two gray borders > framing the PDF content. There are also PDF controls in an overlay. > Rendering the same PDF in an img produced neither of these, so I think a ref > test may be tricky. Agreed, I don't think a ref test is going to go well.
EWS
Comment 10 2023-03-24 22:22:52 PDT
Committed 262109@main (d191671333e1): <https://commits.webkit.org/262109@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 465558 [details].
Note You need to log in before you can comment on or make changes to this bug.