Attachment 342123 Details for Bug 186383 – Fixes the bug

[patch] Fixes the bug

bug-186383-20180606230827.patch (text/plain), 8.38 KB, created by Ryosuke Niwa on 2018-06-06 23:08:27 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-06-06 23:08:27 PDT

Size: 8.38 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 232573)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,19 @@
>+2018-06-06  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 NOBODY (OOPS!).
>+
>+        The release assert was hit because the descendent elemenet iterator, which instantiates ScriptDisallowedScope,
>+        was alive as determinePrimarySnapshottedPlugIn updated the layout via RenderView::hitTest. 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-06  Per Arne Vollan  <pvollan@apple.com>
> 
>         Crash in lambda function WTF::Function<void ()>::CallableWrapper<WebKit::DisplayLink::displayLinkCallback
>Index: Source/WebKit/WebProcess/WebPage/WebPage.cpp
>===================================================================
>--- Source/WebKit/WebProcess/WebPage/WebPage.cpp	(revision 232536)
>+++ Source/WebKit/WebProcess/WebPage/WebPage.cpp	(working copy)
>@@ -5369,44 +5369,49 @@ void WebPage::determinePrimarySnapshotte
> 
>     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;
> 
>@@ -5418,18 +5423,18 @@ void WebPage::determinePrimarySnapshotte
>             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) {
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 232536)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2018-06-06  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 NOBODY (OOPS!).
>+
>+        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-05  Jer Noble  <jer.noble@apple.com>
> 
>         REGRESSION (231817): Videos permanently blank out after switching out of a tab and back in
>Index: LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt
>===================================================================
>--- LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt	(nonexistent)
>+++ LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt	(working copy)
>@@ -0,0 +1,4 @@
>+This tests determining the primary snapshotted plugin does not hit an assertion.
>+
>+PASS
>+
>Index: LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html
>===================================================================
>--- LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html	(nonexistent)
>+++ LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html	(working copy)
>@@ -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>

Flags:

jonlee: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186383: 342123