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
Created attachment 465558 [details] Patch
<rdar://problem/107156793>
rdar://106880773
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*?
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?
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.
(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.
(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.
(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.
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].