From 0cb762ea37cb48b372e841c924e9701461e14371 Mon Sep 17 00:00:00 2001 From: "rniwa@webkit.org" Date: Thu, 7 Jun 2018 18:38:40 +0000 Subject: [PATCH] Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn() https://bugs.webkit.org/show_bug.cgi?id=186383 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 | 13 ++++++++ ...e-primary-snapshotted-plugin-crash-expected.txt | 4 +++ ...determine-primary-snapshotted-plugin-crash.html | 25 +++++++++++++++ Source/WebKit/ChangeLog | 16 ++++++++++ Source/WebKit/WebProcess/WebPage/WebPage.cpp | 37 ++++++++++++---------- 5 files changed, 79 insertions(+), 16 deletions(-) create mode 100644 LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt create mode 100644 LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 46fb8f7..c37dc7e 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,16 @@ +2018-06-07 Ryosuke Niwa + + Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn() + https://bugs.webkit.org/show_bug.cgi?id=186383 + + + 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 [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 index 0000000..0808882 --- /dev/null +++ b/LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash-expected.txt @@ -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 index 0000000..afe52e2 --- /dev/null +++ b/LayoutTests/plugins/snapshotting/determine-primary-snapshotted-plugin-crash.html @@ -0,0 +1,25 @@ + + + +

This tests determining the primary snapshotted plugin does not hit an assertion.

FAIL

+ diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog index 948f13b..bf01846 100644 --- a/Source/WebKit/ChangeLog +++ b/Source/WebKit/ChangeLog @@ -1,3 +1,19 @@ +2018-06-07 Ryosuke Niwa + + Release assert in Document::updateLayout() in WebPage::determinePrimarySnapshottedPlugIn() + https://bugs.webkit.org/show_bug.cgi?id=186383 + + + 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 [CoordGraphics] Fix compilation errors around USE(COORDINATED_GRAPHICS) diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp index 6c365f8..8d37707 100644 --- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp +++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp @@ -5371,44 +5371,49 @@ void WebPage::determinePrimarySnapshottedPlugIn() layoutIfNeeded(); - auto& mainFrame = corePage()->mainFrame(); - if (!mainFrame.view()) - return; - if (!mainFrame.view()->renderView()) + RefPtr 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 candidatePlugIn; unsigned candidatePlugInArea = 0; - for (Frame* frame = &mainFrame; frame; frame = frame->tree().traverseNextRendered()) { + for (RefPtr frame = &corePage()->mainFrame(); frame; frame = frame->tree().traverseNextRendered()) { if (!frame->loader().subframeLoader().containsPlugins()) continue; if (!frame->document() || !frame->view()) continue; + + Vector> nonPlayingPlugInImageElements; for (auto& plugInImageElement : descendantsOfType(*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(*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 = 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(*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) { -- 1.8.3.1