Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlu...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Jun 2018 18:38:40 +0000 (18:38 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Jun 2018 18:38:40 +0000 (18:38 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232591 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt [new file with mode: 0644]
LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebPage/WebPage.cpp

index 46fb8f7..c37dc7e 100644 (file)
@@ -1,3 +1,16 @@
+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>
 
         [GTK][WPE] Start implementing MediaStream API
diff --git a/LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt b/LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt
new file mode 100644 (file)
index 0000000..0808882
--- /dev/null
@@ -0,0 +1,4 @@
+This tests determining the primary snapshotted plugin does not hit an assertion.
+
+PASS
+
diff --git a/LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html b/LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html
new file mode 100644 (file)
index 0000000..afe52e2
--- /dev/null
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests determining the primary snapshotted plugin does not hit an assertion.</p>
+<div id="result">FAIL</div>
+<embed id="testPlugin" src="webkit-test-netscape" type="application/x-webkit-test-netscape"></embed>
+<script>
+if (!window.testRunner)
+    result.textContent = 'This test requires WebKitTestRunner.';
+else {
+    internals.settings.setPlugInSnapshottingEnabled(true);
+    internals.settings.setMaximumPlugInSnapshotAttempts(0);
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+    setTimeout(() => {
+        const testPlugin = document.getElementById("testPlugin");
+        if (internals.isPluginSnapshotted(testPlugin))
+            result.textContent = 'PASS';
+        else
+            result.textContent = 'FAIL - not snapshot';
+        testRunner.notifyDone();
+    }, 500);
+}
+
+</script>
index 948f13b..bf01846 100644 (file)
@@ -1,3 +1,19 @@
+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>
 
         [CoordGraphics] Fix compilation errors around USE(COORDINATED_GRAPHICS)
index 6c365f8..8d37707 100644 (file)
@@ -5371,44 +5371,49 @@ void WebPage::determinePrimarySnapshottedPlugIn()
 
     layoutIfNeeded();
 
-    auto& mainFrame = corePage()->mainFrame();
-    if (!mainFrame.view())
-        return;
-    if (!mainFrame.view()->renderView())
+    RefPtr<FrameView> mainFrameView = corePage()->mainFrame().view();
+    if (!mainFrameView)
         return;
-    RenderView& mainRenderView = *mainFrame.view()->renderView();
 
     IntRect searchRect = IntRect(IntPoint(), corePage()->mainFrame().view()->contentsSize());
     searchRect.intersect(IntRect(IntPoint(), IntSize(primarySnapshottedPlugInSearchLimit, primarySnapshottedPlugInSearchLimit)));
 
     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;
+            nonPlayingPlugInImageElements.append(plugInImageElement);
+        }
 
-            auto pluginRenderer = plugInImageElement.renderer();
+        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;
 
@@ -5420,18 +5425,18 @@ void WebPage::determinePrimarySnapshottedPlugIn()
             inflatedPluginRect.inflateX(xOffset);
             inflatedPluginRect.inflateY(yOffset);
 
-            if (element != &plugInImageElement) {
+            if (element != plugInImageElement.ptr()) {
                 if (!(is<HTMLImageElement>(*element)
                     && inflatedPluginRect.contains(elementRectRelativeToTopDocument)
                     && elementRectRelativeToTopDocument.width() > pluginRenderBox.width() * minimumOverlappingImageToPluginDimensionScale
                     && elementRectRelativeToTopDocument.height() > pluginRenderBox.height() * minimumOverlappingImageToPluginDimensionScale))
                     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);
         }
     }
     if (!candidatePlugIn) {