Crash when breakpoint hit in unload handler
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Feb 2018 22:39:47 +0000 (22:39 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Feb 2018 22:39:47 +0000 (22:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169855
<rdar://problem/28683567>

Source/WebCore:

Reviewed by Daniel Bates.

Test: inspector/debugger/reload-paused.html

CachedRawResource::updateBuffer may generate unload event in client notify callback. If Inspector was
paused, this even would spawn a nested runloop. CachedRawResource::finishLoading would get called in
the nested loop, confusing the DocumentLoader state machine and resulting in crashes later.

* loader/cache/CachedRawResource.cpp:
(WebCore::CachedRawResource::updateBuffer):

- Set a bit when entering the client callback.
- Ensure we don't re-enter updateBuffer.
- If finishLoading got delayed during client callback, do it at the end.

(WebCore::CachedRawResource::finishLoading):

If we are in updateBuffer client callback, save the buffer and bail out.

* loader/cache/CachedRawResource.h:

LayoutTests:

Reviewed by Daniel Bates and Joseph Pecoraro.

* inspector/debugger/reload-paused-expected.txt: Added.
* inspector/debugger/reload-paused.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/inspector/debugger/reload-paused-expected.txt [new file with mode: 0644]
LayoutTests/inspector/debugger/reload-paused.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedRawResource.cpp
Source/WebCore/loader/cache/CachedRawResource.h

index 68dbd3d..72b539e 100644 (file)
@@ -1,3 +1,14 @@
+2018-02-13  Antti Koivisto  <antti@apple.com>
+
+        Crash when breakpoint hit in unload handler
+        https://bugs.webkit.org/show_bug.cgi?id=169855
+        <rdar://problem/28683567>
+
+        Reviewed by Daniel Bates and Joseph Pecoraro.
+
+        * inspector/debugger/reload-paused-expected.txt: Added.
+        * inspector/debugger/reload-paused.html: Added.
+
 2018-02-13  Nan Wang  <n_wang@apple.com>
 
         AX: Remove AccessibleNode class
diff --git a/LayoutTests/inspector/debugger/reload-paused-expected.txt b/LayoutTests/inspector/debugger/reload-paused-expected.txt
new file mode 100644 (file)
index 0000000..ab51f3d
--- /dev/null
@@ -0,0 +1,8 @@
+main frame - has 1 onunload handler(s)
+main frame - has 1 onunload handler(s)
+Test that reloading a paused page doesn't crash.
+
+
+== Running test suite: ReloadPaused
+-- Running test case: ReloadPausedNoCrash
+
diff --git a/LayoutTests/inspector/debugger/reload-paused.html b/LayoutTests/inspector/debugger/reload-paused.html
new file mode 100644 (file)
index 0000000..39c4de0
--- /dev/null
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
+<script src="resources/break-on-exception-tests.js"></script>
+<script>
+function unloadHandler()
+{
+    debugger;
+}
+
+function test()
+{
+    WI.debuggerManager.allExceptionsBreakpoint.disabled = false;
+
+    let suite = InspectorTest.createAsyncSuite("ReloadPaused");
+
+    suite.addTestCase({
+       name: "ReloadPausedNoCrash",
+       async test() {
+           InspectorTest.reloadPage();
+           await WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused);
+           await WI.debuggerManager.resume();
+       }
+   });
+
+   suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body onload="runTest()" onunload="unloadHandler()">
+<p>Test that reloading a paused page doesn't crash.</p>
+</body>
+</html>
index 03b1b86..f3547cc 100644 (file)
@@ -1,3 +1,30 @@
+2018-02-13  Antti Koivisto  <antti@apple.com>
+
+        Crash when breakpoint hit in unload handler
+        https://bugs.webkit.org/show_bug.cgi?id=169855
+        <rdar://problem/28683567>
+
+        Reviewed by Daniel Bates.
+
+        Test: inspector/debugger/reload-paused.html
+
+        CachedRawResource::updateBuffer may generate unload event in client notify callback. If Inspector was
+        paused, this even would spawn a nested runloop. CachedRawResource::finishLoading would get called in
+        the nested loop, confusing the DocumentLoader state machine and resulting in crashes later.
+
+        * loader/cache/CachedRawResource.cpp:
+        (WebCore::CachedRawResource::updateBuffer):
+
+        - Set a bit when entering the client callback.
+        - Ensure we don't re-enter updateBuffer.
+        - If finishLoading got delayed during client callback, do it at the end.
+
+        (WebCore::CachedRawResource::finishLoading):
+
+        If we are in updateBuffer client callback, save the buffer and bail out.
+
+        * loader/cache/CachedRawResource.h:
+
 2018-02-13  Zalan Bujtas  <zalan@apple.com>
 
         [RenderTreeBuilder] Move RenderBlockFlow::takeChild() to RenderTreeBuilder
index 98ef34d..e7dbfcd 100644 (file)
@@ -33,6 +33,7 @@
 #include "SharedBuffer.h"
 #include "SubresourceLoader.h"
 #include <wtf/CompletionHandler.h>
+#include <wtf/SetForScope.h>
 #include <wtf/text/StringView.h>
 
 namespace WebCore {
@@ -55,22 +56,32 @@ std::optional<SharedBufferDataView> CachedRawResource::calculateIncrementalDataC
 
 void CachedRawResource::updateBuffer(SharedBuffer& data)
 {
+    // Skip any updateBuffers triggered from nested runloops. We'll have the complete buffer in finishLoading.
+    if (m_inIncrementalDataNotify)
+        return;
+
     CachedResourceHandle<CachedRawResource> protectedThis(this);
     ASSERT(dataBufferingPolicy() == BufferData);
     m_data = &data;
 
     auto incrementalData = calculateIncrementalDataChunk(&data);
     setEncodedSize(data.size());
-    if (incrementalData)
+    if (incrementalData) {
+        SetForScope<bool> notifyScope(m_inIncrementalDataNotify, true);
         notifyClientsDataWasReceived(incrementalData->data(), incrementalData->size());
+    }
+
     if (dataBufferingPolicy() == DoNotBufferData) {
         if (m_loader)
             m_loader->setDataBufferingPolicy(DoNotBufferData);
         clear();
-        return;
-    }
+    } else
+        CachedResource::updateBuffer(data);
 
-    CachedResource::updateBuffer(data);
+    if (m_delayedFinishLoading) {
+        auto delayedFinishLoading = std::exchange(m_delayedFinishLoading, std::nullopt);
+        finishLoading(delayedFinishLoading->buffer.get());
+    }
 }
 
 void CachedRawResource::updateData(const char* data, unsigned length)
@@ -82,6 +93,12 @@ void CachedRawResource::updateData(const char* data, unsigned length)
 
 void CachedRawResource::finishLoading(SharedBuffer* data)
 {
+    if (m_inIncrementalDataNotify) {
+        // We may get here synchronously from updateBuffer() if the callback there ends up spinning a runloop.
+        // In that case delay the call.
+        m_delayedFinishLoading = std::make_optional(DelayedFinishLoading { data });
+        return;
+    };
     CachedResourceHandle<CachedRawResource> protectedThis(this);
     DataBufferingPolicy dataBufferingPolicy = this->dataBufferingPolicy();
     if (dataBufferingPolicy == BufferData) {
index 9d0b08a..f069487 100644 (file)
@@ -75,6 +75,7 @@ private:
 
     unsigned long m_identifier;
     bool m_allowEncodedDataReplacement;
+    bool m_inIncrementalDataNotify { false };
 
     struct RedirectPair {
     public:
@@ -89,6 +90,11 @@ private:
     };
 
     Vector<RedirectPair> m_redirectChain;
+
+    struct DelayedFinishLoading {
+        RefPtr<SharedBuffer> buffer;
+    };
+    std::optional<DelayedFinishLoading> m_delayedFinishLoading;
 };
 
 } // namespace WebCore