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

Description Tyler Wilcock 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
Comment 1 Tyler Wilcock 2023-03-23 15:28:07 PDT 
Created attachment 465558 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2023-03-23 15:35:56 PDT 
<rdar://problem/107156793>
Comment 3 Tyler Wilcock 2023-03-23 15:36:09 PDT 
rdar://106880773
Comment 4 Tim Horton 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*?
Comment 5 Simon Fraser (smfr) 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?
Comment 6 Chris Dumez 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.
Comment 7 Tyler Wilcock 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.
Comment 8 Tyler Wilcock 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.
Comment 9 Tim Horton 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.
Comment 10 EWS 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].