When doing incremental PDF loading, gather PDF scripts to run on a background thread <rdar://problem/60422027>
Created attachment 393500 [details] Patch
Comment on attachment 393500 [details] Patch Looks like this needs a rebase to run in EWS.
(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
Created attachment 393510 [details] Patch
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?
(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.
Created attachment 393542 [details] Patch
Created attachment 393545 [details] Patch
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.
(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)
> 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.)
Created attachment 393590 [details] Patch
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?
(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.
Created attachment 393592 [details] PFL
Comment on attachment 393592 [details] PFL Clearing flags on attachment: 393592 Committed r258474: <https://trac.webkit.org/changeset/258474>
All reviewed patches have been landed. Closing bug.