Title: [228435] trunk
- Revision
- 228435
- Author
- an...@apple.com
- Date
- 2018-02-13 14:39:47 -0800 (Tue, 13 Feb 2018)
Log Message
Crash when breakpoint hit in unload handler
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (228434 => 228435)
--- trunk/LayoutTests/ChangeLog 2018-02-13 22:36:15 UTC (rev 228434)
+++ trunk/LayoutTests/ChangeLog 2018-02-13 22:39:47 UTC (rev 228435)
@@ -1,3 +1,14 @@
+2018-02-13 Antti Koivisto <an...@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_w...@apple.com>
AX: Remove AccessibleNode class
Added: trunk/LayoutTests/inspector/debugger/reload-paused-expected.txt (0 => 228435)
--- trunk/LayoutTests/inspector/debugger/reload-paused-expected.txt (rev 0)
+++ trunk/LayoutTests/inspector/debugger/reload-paused-expected.txt 2018-02-13 22:39:47 UTC (rev 228435)
@@ -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
+
Added: trunk/LayoutTests/inspector/debugger/reload-paused.html (0 => 228435)
--- trunk/LayoutTests/inspector/debugger/reload-paused.html (rev 0)
+++ trunk/LayoutTests/inspector/debugger/reload-paused.html 2018-02-13 22:39:47 UTC (rev 228435)
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<script src=""
+<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>
Modified: trunk/Source/WebCore/ChangeLog (228434 => 228435)
--- trunk/Source/WebCore/ChangeLog 2018-02-13 22:36:15 UTC (rev 228434)
+++ trunk/Source/WebCore/ChangeLog 2018-02-13 22:39:47 UTC (rev 228435)
@@ -1,3 +1,30 @@
+2018-02-13 Antti Koivisto <an...@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 <za...@apple.com>
[RenderTreeBuilder] Move RenderBlockFlow::takeChild() to RenderTreeBuilder
Modified: trunk/Source/WebCore/loader/cache/CachedRawResource.cpp (228434 => 228435)
--- trunk/Source/WebCore/loader/cache/CachedRawResource.cpp 2018-02-13 22:36:15 UTC (rev 228434)
+++ trunk/Source/WebCore/loader/cache/CachedRawResource.cpp 2018-02-13 22:39:47 UTC (rev 228435)
@@ -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,6 +56,10 @@
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;
@@ -61,16 +66,22 @@
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);
+
+ if (m_delayedFinishLoading) {
+ auto delayedFinishLoading = std::exchange(m_delayedFinishLoading, std::nullopt);
+ finishLoading(delayedFinishLoading->buffer.get());
}
-
- CachedResource::updateBuffer(data);
}
void CachedRawResource::updateData(const char* data, unsigned length)
@@ -82,6 +93,12 @@
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) {
Modified: trunk/Source/WebCore/loader/cache/CachedRawResource.h (228434 => 228435)
--- trunk/Source/WebCore/loader/cache/CachedRawResource.h 2018-02-13 22:36:15 UTC (rev 228434)
+++ trunk/Source/WebCore/loader/cache/CachedRawResource.h 2018-02-13 22:39:47 UTC (rev 228435)
@@ -75,6 +75,7 @@
unsigned long m_identifier;
bool m_allowEncodedDataReplacement;
+ bool m_inIncrementalDataNotify { false };
struct RedirectPair {
public:
@@ -89,6 +90,11 @@
};
Vector<RedirectPair> m_redirectChain;
+
+ struct DelayedFinishLoading {
+ RefPtr<SharedBuffer> buffer;
+ };
+ std::optional<DelayedFinishLoading> m_delayedFinishLoading;
};
} // namespace WebCore
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
