Changeset 264945 in webkit

Timestamp:

Jul 27, 2020, 2:57:15 PM (6 years ago)

Author:

timothy_horton@apple.com

Message:

Occasional crashes under PDFPlugin::createScrollbar and PDFPlugin::updateScrollbars
​https://bugs.webkit.org/show_bug.cgi?id=214845
<rdar://problem/59293598>

Reviewed by Brady Eidson.

No new tests; timing is such that I can't reproduce without inserting
intentional delays into the main thread hops, which is further than
I'm willing to go for a test.

This is a speculative fix due to the aforementioned reproducibility issue.

• WebProcess/Plugins/PDF/PDFPlugin.h:
• WebProcess/Plugins/PDF/PDFPlugin.mm:

(WebKit::PDFPlugin::destroy):
Keep track of when destroy() has been called.

(WebKit::PDFPlugin::receivedNonLinearizedPDFSentinel):
(WebKit::PDFPlugin::threadEntry):
If the PDFPlugin is destroyed (explicit destroy(), not the destructor, which won't run since we're protecting this)
during the window in which the PDF thread is dispatching to the main thread, just ignore
the callback. We don't want to run it, because many of the things we depend on (like PluginView)
are disconnected at this point.

(WebKit::PDFPlugin::scrollbarStyleChanged):
It appears that in some much rarer case, even before the PDF thread existed, WebScrollerImpPairDelegate could
call us back after being destroy()'d but before being deallocated. Use the new bit to guard scrollbarStyleChanged().

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• WebProcess/Plugins/PDF/PDFPlugin.h (modified) (1 diff)
• WebProcess/Plugins/PDF/PDFPlugin.mm (modified) (4 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-07-27  Tim Horton  <timothy_horton@apple.com>
+
+        Occasional crashes under PDFPlugin::createScrollbar and PDFPlugin::updateScrollbars
+        https://bugs.webkit.org/show_bug.cgi?id=214845
+        <rdar://problem/59293598>
+
+        Reviewed by Brady Eidson.
+
+        No new tests; timing is such that I can't reproduce without inserting
+        intentional delays into the main thread hops, which is further than
+        I'm willing to go for a test.
+
+        This is a speculative fix due to the aforementioned reproducibility issue.
+
+        * WebProcess/Plugins/PDF/PDFPlugin.h:
+        * WebProcess/Plugins/PDF/PDFPlugin.mm:
+        (WebKit::PDFPlugin::destroy):
+        Keep track of when destroy() has been called.
+
+        (WebKit::PDFPlugin::receivedNonLinearizedPDFSentinel):
+        (WebKit::PDFPlugin::threadEntry):
+        If the PDFPlugin is destroyed (explicit destroy(), not the destructor, which won't run since we're protecting `this`)
+        during the window in which the PDF thread is dispatching to the main thread, just ignore
+        the callback. We don't want to run it, because many of the things we depend on (like PluginView)
+        are disconnected at this point.
+
+        (WebKit::PDFPlugin::scrollbarStyleChanged):
+        It appears that in some much rarer case, even before the PDF thread existed, WebScrollerImpPairDelegate could
+        call us back after being destroy()'d but before being deallocated. Use the new bit to guard scrollbarStyleChanged().
+
 2020-07-27  Megan Gardner  <megan_gardner@apple.com>
 

trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h

@@ -332 +332 @@
 
     bool m_documentFinishedLoading { false };
+    bool m_hasBeenDestroyed { false };
     unsigned m_firstPageHeight { 0 };
     WebCore::IntSize m_pdfDocumentSize; // All pages, including gaps.

trunk/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm

@@ -711 +711 @@
 #endif
         callOnMainThread([this, protectedThis = makeRef(*this)] {
+            if (m_hasBeenDestroyed)
+                return;
             receivedNonLinearizedPDFSentinel();
         });
@@ -886 +888 @@
         if (m_incrementalPDFLoadingEnabled) {
             callOnMainThread([this] {
+                if (m_hasBeenDestroyed)
+                    return;
                 adoptBackgroundThreadDocument();
             });
@@ -1431 +1435 @@
 {
     if (!forceUpdate)
+        return;
+
+    if (m_hasBeenDestroyed)
         return;
 
@@ -1791 +1798 @@
 void PDFPlugin::destroy()
 {
+    m_hasBeenDestroyed = true;
     m_documentFinishedLoading = true;