Attachment 241929 Details for Bug 138914 – Patch

[patch] Patch

bug-138914-20141120002321.patch (text/plain), 6.34 KB, created by Chris Dumez on 2014-11-20 00:23:07 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2014-11-20 00:23:07 PST

Size: 6.34 KB

patch

obsolete

>Subversion Revision: 176378
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f2bdae313e5443d51ebfd40f7befa67fc7c05416..0ca0249ca9562f20a5e79e3310353bdbc4bee0f5 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2014-11-20  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash when destroying a Document that has a throttled timer still running
>+        https://bugs.webkit.org/show_bug.cgi?id=138914
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Upon destruction, a throttled DOMTimer whose interval depends on
>+        viewport changes will try to unregister itself from the view. It gets
>+        the view pointer from its Document. However, scriptExecutionContext()
>+        can return null if the Document is being destroyed (i.e. ~DOMTimer()
>+        is called from ~ScriptExecutionContext(), as the ScriptExecutionContext
>+        owns the DOMTimer).
>+
>+        This patch adds a null check for scriptExecutionContext() in the
>+        DOMTimer destructor to avoid this issue.
>+
>+        Test: fast/dom/throttled-timer-running-on-document-destruction.html
>+
>+        * page/DOMTimer.cpp:
>+        (WebCore::DOMTimer::~DOMTimer):
>+
>+        (WebCore::DOMTimer::unregisterForViewportChanges):
>+        Add assertion to make sure scriptExecutionContext() does not return
>+        null.
>+
>+        (WebCore::DOMTimerFireState::setChangedStyleOfElementOutsideViewport): Deleted.
>+        Killed this function as this was dead code.
>+
> 2014-11-19  Andreas Kling  <akling@apple.com>
> 
>         Remove "document has no ::before and/or ::after rules" optimization.
>diff --git a/Source/WebCore/page/DOMTimer.cpp b/Source/WebCore/page/DOMTimer.cpp
>index e3c1ad2748bdbbf5e8bcec74ba6d17fd74a26c03..b4362ce1f18fc1ab7b51ec569ad261844614a4e6 100644
>--- a/Source/WebCore/page/DOMTimer.cpp
>+++ b/Source/WebCore/page/DOMTimer.cpp
>@@ -99,11 +99,6 @@ public:
>         return document && document->domTreeVersion() != m_initialDOMTreeVersion;
>     }
> 
>-    void setChangedStyleOfElementOutsideViewport(StyledElement& element)
>-    {
>-        m_elementsChangedOutsideViewport.add(&element);
>-    }
>-
>     void elementsChangedOutsideViewport(Vector<RefPtr<StyledElement>>& elements) const
>     {
>         copyToVector(m_elementsChangedOutsideViewport, elements);
>@@ -209,7 +204,9 @@ DOMTimer::DOMTimer(ScriptExecutionContext& context, std::unique_ptr<ScheduledAct
> 
> DOMTimer::~DOMTimer()
> {
>-    if (isIntervalDependentOnViewport())
>+    // If the ScriptExecutionContext has already been destroyed, there is
>+    // no need to stop listening for viewport changes.
>+    if (scriptExecutionContext() && isIntervalDependentOnViewport())
>         unregisterForViewportChanges();
> }
> 
>@@ -443,6 +440,7 @@ void DOMTimer::registerForViewportChanges()
> 
> void DOMTimer::unregisterForViewportChanges()
> {
>+    ASSERT(scriptExecutionContext());
>     if (auto* frameView = downcast<Document>(*scriptExecutionContext()).view())
>         frameView->unregisterThrottledDOMTimer(this);
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 00b0b3a2e5833fdf04002cb5e72fb96bb39f09c3..c02797a4752bd2b42e9872f8c80a8d7634fed7b6 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2014-11-20  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash when destroying a Document that has a throttled timer still running
>+        https://bugs.webkit.org/show_bug.cgi?id=138914
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a layout test to test the case where a Document gets destroyed while
>+        throttled timer is still running.
>+
>+        * fast/dom/resources/frame-with-throttled-timer.html: Added.
>+        * fast/dom/throttled-timer-running-on-document-destruction-expected.txt: Added.
>+        * fast/dom/throttled-timer-running-on-document-destruction.html: Added.
>+
> 2014-11-19  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>         [OS X] Upright vertical text is completely broken for multi-code-unit codepoints
>diff --git a/LayoutTests/fast/dom/resources/frame-with-throttled-timer.html b/LayoutTests/fast/dom/resources/frame-with-throttled-timer.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..ef32dad27a328a80fd9b7cbd8bde0af605b1b049
>--- /dev/null
>+++ b/LayoutTests/fast/dom/resources/frame-with-throttled-timer.html
>@@ -0,0 +1,14 @@
>+<!DOCTYPE html>
>+<body>
>+<iframe id="testFrame"></iframe>
>+<script>
>+document.getElementById('testFrame').contentDocument.body.innerHTML = "<div id='testElement' style='display: none'>TEST</div>"
>+
>+setInterval(function() {
>+  // Change the style of a display:none element.
>+  var testFrame = document.getElementById("testFrame");
>+  var testElement = testFrame.contentDocument.getElementById("testElement");
>+  testElement.style["left"] = "" + Math.floor((Math.random() * 10) + 1) + "px";
>+}, 5);
>+</script>
>+</body>
>diff --git a/LayoutTests/fast/dom/throttled-timer-running-on-document-destruction-expected.txt b/LayoutTests/fast/dom/throttled-timer-running-on-document-destruction-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..76b79c7accf95100ea9cccbed4c7a9f3cef1625b
>--- /dev/null
>+++ b/LayoutTests/fast/dom/throttled-timer-running-on-document-destruction-expected.txt
>@@ -0,0 +1,10 @@
>+Test that we don't crash if a throttled timer is still running when the document is destroyed.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS Did not crash.
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/fast/dom/throttled-timer-running-on-document-destruction.html b/LayoutTests/fast/dom/throttled-timer-running-on-document-destruction.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..b28fde44007dac73dc006adfbeb96fa306c3bc27
>--- /dev/null
>+++ b/LayoutTests/fast/dom/throttled-timer-running-on-document-destruction.html
>@@ -0,0 +1,22 @@
>+<!DOCTYPE html>
>+<body>
>+<script src="../../resources/js-test-pre.js"></script>
>+<iframe id="testFrame" src="resources/frame-with-throttled-timer.html"></iframe>
>+
>+<script>
>+description("Test that we don't crash if a throttled timer is still running when the document is destroyed.");
>+jsTestIsAsync = true;
>+
>+function removeFrame()
>+{
>+  document.body.removeChild(document.getElementById("testFrame"));
>+  gc();
>+  testPassed("Did not crash.");
>+  finishJSTest();
>+}
>+
>+setTimeout(removeFrame, 300);
>+
>+</script>
>+<script src="../../resources/js-test-post.js"></script>
>+</body>

Actions: View | Formatted Diff | Diff

Attachments on bug 138914: 241929