Changeset 232591 in webkit

Timestamp:

Jun 7, 2018 11:38:40 AM (6 years ago)

Author:

rniwa@webkit.org

Message:

Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn()
​https://bugs.webkit.org/show_bug.cgi?id=186383
<rdar://problem/40849498>

Reviewed by Jon Lee.

Source/WebKit:

The release assert was hit because the descendent elemenet iterator, which instantiates ScriptDisallowedScope,
was alive as determinePrimarySnapshottedPlugIn invoked Document::updateLayout. Avoid this by copying
the list of plugin image elements into a vector first.

• WebProcess/WebPage/WebPage.cpp:

(WebKit::WebPage::determinePrimarySnapshottedPlugIn): Fixed the release assert, and deployed Ref and RefPtr
to make this code safe.

LayoutTests:

Added a regression test.

• plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt: Added.
• plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt (added)
• LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html (added)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/WebProcess/WebPage/WebPage.cpp (modified) (4 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-06-07  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn()
+        https://bugs.webkit.org/show_bug.cgi?id=186383
+        <rdar://problem/40849498>
+
+        Reviewed by Jon Lee.
+
+        Added a regression test.
+
+        * plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt: Added.
+        * plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html: Added.
+
 2018-06-07  Thibault Saunier  <tsaunier@igalia.com>
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2018-06-07  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn()
+        https://bugs.webkit.org/show_bug.cgi?id=186383
+        <rdar://problem/40849498>
+
+        Reviewed by Jon Lee.
+
+        The release assert was hit because the descendent elemenet iterator, which instantiates ScriptDisallowedScope,
+        was alive as determinePrimarySnapshottedPlugIn invoked Document::updateLayout. Avoid this by copying
+        the list of plugin image elements into a vector first.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::determinePrimarySnapshottedPlugIn): Fixed the release assert, and deployed Ref and RefPtr
+        to make this code safe.
+
 2018-06-07  Don Olmstead  <don.olmstead@sony.com>
 

trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

@@ -5372 +5372 @@
     layoutIfNeeded();
 
-    auto& mainFrame = corePage()->mainFrame();
-    if (!mainFrame.view())
-        return;
-    if (!mainFrame.view()->renderView())
-        return;
-    RenderView& mainRenderView = *mainFrame.view()->renderView();
+    RefPtr<FrameView> mainFrameView = corePage()->mainFrame().view();
+    if (!mainFrameView)
+        return;
 
     IntRect searchRect = IntRect(IntPoint(), corePage()->mainFrame().view()->contentsSize());
@@ -5384 +5381 @@
     HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowChildFrameContent | HitTestRequest::IgnoreClipping | HitTestRequest::DisallowUserAgentShadowContent);
 
-    HTMLPlugInImageElement* candidatePlugIn = nullptr;
+    RefPtr<HTMLPlugInImageElement> candidatePlugIn;
     unsigned candidatePlugInArea = 0;
 
-    for (Frame* frame = &mainFrame; frame; frame = frame->tree().traverseNextRendered()) {
+    for (RefPtr<Frame> frame = &corePage()->mainFrame(); frame; frame = frame->tree().traverseNextRendered()) {
         if (!frame->loader().subframeLoader().containsPlugins())
             continue;
         if (!frame->document() || !frame->view())
             continue;
+
+        Vector<Ref<HTMLPlugInImageElement>> nonPlayingPlugInImageElements;
         for (auto& plugInImageElement : descendantsOfType<HTMLPlugInImageElement>(*frame->document())) {
             if (plugInImageElement.displayState() == HTMLPlugInElement::Playing)
                 continue;
-
-            auto pluginRenderer = plugInImageElement.renderer();
+            nonPlayingPlugInImageElements.append(plugInImageElement);
+        }
+
+        for (auto& plugInImageElement : nonPlayingPlugInImageElements) {
+            auto pluginRenderer = plugInImageElement->renderer();
             if (!pluginRenderer || !pluginRenderer->isBox())
                 continue;
             auto& pluginRenderBox = downcast<RenderBox>(*pluginRenderer);
-            if (!plugInIntersectsSearchRect(plugInImageElement))
+            if (!plugInIntersectsSearchRect(plugInImageElement.get()))
                 continue;
 
-            IntRect plugInRectRelativeToView = plugInImageElement.clientRect();
-            ScrollPosition scrollPosition = mainFrame.view()->documentScrollPositionRelativeToViewOrigin();
+            IntRect plugInRectRelativeToView = plugInImageElement->clientRect();
+            ScrollPosition scrollPosition = mainFrameView->documentScrollPositionRelativeToViewOrigin();
             IntRect plugInRectRelativeToTopDocument(plugInRectRelativeToView.location() + scrollPosition, plugInRectRelativeToView.size());
             HitTestResult hitTestResult(plugInRectRelativeToTopDocument.center());
-            mainRenderView.hitTest(request, hitTestResult);
-
-            Element* element = hitTestResult.targetElement();
+
+            if (!mainFrameView->renderView())
+                return;
+            mainFrameView->renderView()->hitTest(request, hitTestResult);
+
+            RefPtr<Element> element = hitTestResult.targetElement();
             if (!element)
                 continue;
@@ -5421 +5426 @@
             inflatedPluginRect.inflateY(yOffset);
 
-            if (element != &plugInImageElement) {
+            if (element != plugInImageElement.ptr()) {
                 if (!(is<HTMLImageElement>(*element)
                     && inflatedPluginRect.contains(elementRectRelativeToTopDocument)
@@ -5428 +5433 @@
                     continue;
                 LOG(Plugins, "Primary Plug-In Detection: Plug-in is hidden by an image that is roughly aligned with it, autoplaying regardless of whether or not it's actually the primary plug-in.");
-                plugInImageElement.restartSnapshottedPlugIn();
+                plugInImageElement->restartSnapshottedPlugIn();
             }
 
             if (plugInIsPrimarySize(plugInImageElement, candidatePlugInArea))
-                candidatePlugIn = &plugInImageElement;
+                candidatePlugIn = WTFMove(plugInImageElement);
         }
     }