Bug 209063 – Gather PDF scripts to run on a background thread

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

When doing incremental PDF loading, gather PDF scripts to run on a background thread

<rdar://problem/60422027>

[Brady Eidson]

Created attachment 393500 [details]
Patch

[Geoffrey Garen]

Comment on attachment 393500 [details]
Patch

Looks like this needs a rebase to run in EWS.

[Brady Eidson]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 393500 [details]
> Patch
> 
> Looks like this needs a rebase to run in EWS.

Yup, confirming I build locally before I upload

[Brady Eidson]

Created attachment 393510 [details]
Patch

[Geoffrey Garen]

Comment on attachment 393510 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393510&action=review

> Source/WebKit/ChangeLog:3
> +        Gather PDF scripts to run on a background thread.

Why do we need to do this? Is it because we might block if the PDF is still loading?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1563
> +void PDFPlugin::maybeRunScriptsInPDFDocument()

=> runScriptsInPDFDocumentIfNeeded

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1572
> +    auto completionHandler = [this, protectedThis = makeRef(*this)] (Vector<RetainPtr<CFStringRef>>& scripts) mutable {

Let's make this Vector& a Vector&& -- that's a better proof of thread safety, I think, since it means you're the single owner.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1576
> +        JSGlobalContextRef ctx = JSGlobalContextCreate(0);

nullptr

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1579
> +            JSEvaluateScript(ctx, OpaqueJSString::tryCreate(script.get()).get(), jsPDFDoc, nullptr, 0, nullptr);

0 => 1 (it's a line number)

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1587
> +    auto& rawQueue = scriptUtilityQueue.get();
> +    auto documentRef = [m_pdfDocument documentRef];
> +    rawQueue.dispatch([scriptUtilityQueue = WTFMove(scriptUtilityQueue), completionHandler = WTFMove(completionHandler), documentRef] () mutable {

Can we do scriptUtilityQueue-> instead of rawQueue.? Less verbose if so.

Can we capture a RetainPtr instead of (what appears to be) a raw pointer?

[Brady Eidson]

(In reply to Geoffrey Garen from comment #5)
> Comment on attachment 393510 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393510&action=review
> 
> > Source/WebKit/ChangeLog:3
> > +        Gather PDF scripts to run on a background thread.
> 
> Why do we need to do this? Is it because we might block if the PDF is still
> loading?

Will clarify in new ChangeLog

> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1563
> > +void PDFPlugin::maybeRunScriptsInPDFDocument()
> 
> => runScriptsInPDFDocumentIfNeeded
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1572
> > +    auto completionHandler = [this, protectedThis = makeRef(*this)] (Vector<RetainPtr<CFStringRef>>& scripts) mutable {
> 
> Let's make this Vector& a Vector&& -- that's a better proof of thread
> safety, I think, since it means you're the single owner.
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1576
> > +        JSGlobalContextRef ctx = JSGlobalContextCreate(0);
> 
> nullptr
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1579
> > +            JSEvaluateScript(ctx, OpaqueJSString::tryCreate(script.get()).get(), jsPDFDoc, nullptr, 0, nullptr);
> 
> 0 => 1 (it's a line number)
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1587
> > +    auto& rawQueue = scriptUtilityQueue.get();
> > +    auto documentRef = [m_pdfDocument documentRef];
> > +    rawQueue.dispatch([scriptUtilityQueue = WTFMove(scriptUtilityQueue), completionHandler = WTFMove(completionHandler), documentRef] () mutable {
> 
> Can we do scriptUtilityQueue-> instead of rawQueue.? Less verbose if so.

No, because the WTFMove for the capture happens before the call.

> Can we capture a RetainPtr instead of (what appears to be) a raw pointer?

In practice not necessary because the m_pdfDocument will always be alive because the PDFPlugin will be too.

But adding it makes that more explicit, so I will.

[Brady Eidson]

Created attachment 393542 [details]
Patch

[Brady Eidson]

Created attachment 393545 [details]
Patch

[Geoffrey Garen]

Comment on attachment 393545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393545&action=review

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1453
> +    if (m_documentFinishedLoading)
> +        runScriptsInPDFDocumentIfNeeded();

Can we just run scripts at the end of pdfDocumentDidLoad() in all cases?

That seems like the most natural time. And it removes the complexity of tracking whether we have run scripts already or not. If it's sometimes not possible to run scripts at the end of pdfDocumentDidLoad(), then I wonder why it's sometimes OK.

[Brady Eidson]

(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 393545 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393545&action=review
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1453
> > +    if (m_documentFinishedLoading)
> > +        runScriptsInPDFDocumentIfNeeded();
> 
> Can we just run scripts at the end of pdfDocumentDidLoad() in all cases?
> 
> That seems like the most natural time. And it removes the complexity of
> tracking whether we have run scripts already or not. If it's sometimes not
> possible to run scripts at the end of pdfDocumentDidLoad(), then I wonder
> why it's sometimes OK.

pdfDocumentDidLoad() is an old method name from the "synchronously create the document at the end" days.

It could be renamed to "documentDataDidFinishLoading()"

You need two things to be true to run scripts:
1 - You need all document data to have finished loading
2 - The PDFDocument needs to have been created.

In the new model, the main thread *can* finish loading all of the data for the document, while the original background PDF thread might not have finished creating the PDFDocument yet.

That's why there are two places that "runScriptsInPDFDocumentIfNeeded":
1 - When the document data finishes loading (assuming the PDFDocument has also been created)
2 - When the PDFDocument has been created (assuming we also have all the document data)

[Geoffrey Garen]

> You need two things to be true to run scripts:
> 1 - You need all document data to have finished loading
> 2 - The PDFDocument needs to have been created.

Given these requirements, how about this design:

1. Change runScriptsInPDFDocument() to tryRunScriptsInPDFDocument()

1a. tryRunScriptsInPDFDocument returns early if m_pdfDocument is null or m_documentFinishedLoading is false

2. PDFPlugin::pdfDocumentDidLoad() calls tryRunScriptsInPDFDocument() unconditionally at the end

3. adoptBackgroundThreadDocument() calls tryRunScriptsInPDFDocument() unconditionally at the end

I think this design might be better because I think it directly encodes the requirements you've described: tryRunScriptsInPDFDocument() only runs if both conditions are true, and each function that changes one of the conditions calls tryRunScriptsInPDFDocument() directly. It also avoids an extra piece of state.

(The patch you've written is good at demonstrating that it never runs scripts twice, but not as good at demonstrating that it runs scripts more than zero times.)

[Brady Eidson]

Created attachment 393590 [details]
Patch

[Geoffrey Garen]

Comment on attachment 393590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393590&action=review

r=me

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1564
> +    ASSERT(m_documentFinishedLoading);

I think this assert is no longer valid?

[Brady Eidson]

(In reply to Geoffrey Garen from comment #13)
> Comment on attachment 393590 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393590&action=review
> 
> r=me
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1564
> > +    ASSERT(m_documentFinishedLoading);
> 
> I think this assert is no longer valid?

Amazing you called that out right after I ran into it in testing.

[Brady Eidson]

Created attachment 393592 [details]
PFL

[WebKit Commit Bot]

Comment on attachment 393592 [details]
PFL

Clearing flags on attachment: 393592

Committed r258474: <https://trac.webkit.org/changeset/258474>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.