Merge r222097 - Make DocumentLoader a FrameDestructionObserver

https://bugs.webkit.org/show_bug.cgi?id=176364 <rdar://problem/34254780> Reviewed by Alex Christensen. The DocumentLoader needs to know when its Frame is destroyed so that it can perform properly cleanup. Test: fast/events/beforeunload-dom-manipulation-crash.html * loader/DocumentLoader.cpp: (WebCore::DocumentLoader::DocumentLoader): Call FrameDestructionObserver constructor. (WebCore::DocumentLoader::responseReceived): Drive-by fix. Make sure the current object is valid during the callback. (WebCore::DocumentLoader::attachToFrame): Use FrameDestructionObserver::observerFrame rather than setting the m_frame variable directly. (WebCore::DocumentLoader::detachFromFrame): Ditto. * loader/DocumentLoader.h: (WebCore::DocumentLoader::frame const): Deleted, as this is provided by the FrameDestructionObserver. Tools: Provide mechanism to immediately end tests https://bugs.webkit.org/show_bug.cgi?id=176364 <rdar://problem/34254780> Reviewed by Alex Christensen. WebKitTestRunner does not output state if the top loading frame has not been removed. This prevents some tests that attempt to exercise failed load state from working properly. This change adds a new 'forceImmediateCompletion' handler for DumpRenderTree and WebKitTestRunner so that we can properly test these conditions. * DumpRenderTree/TestRunner.h: * DumpRenderTree/mac/TestRunnerMac.mm: (TestRunner::forceImmediateCompletion): Added. * DumpRenderTree/win/TestRunnerWin.cpp: (TestRunner::forceImmediateCompletion): Ditto. * WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl: * WebKitTestRunner/InjectedBundle/TestRunner.cpp: (WTR::TestRunner::forceImmediateCompletion): Ditto. * WebKitTestRunner/InjectedBundle/TestRunner.h: LayoutTests: Make DocumentLoader a FrameDestructionObserver https://bugs.webkit.org/show_bug.cgi?id=176364 <rdar://problem/34254780> Reviewed by Alex Christensen. * fast/events/beforeunload-dom-manipulation-crash-expected.txt: Added. * fast/events/beforeunload-dom-manipulation-crash.html: Added.
WebKit · Oct 16, 2017 · 59f7b5a · 59f7b5a
1 parent 49c99a0
commit 59f7b5a
Show file tree

Hide file tree

Showing 13 changed files with 128 additions and 8 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2017-09-15  Brent Fulgham  <bfulgham@apple.com>
+
+        Make DocumentLoader a FrameDestructionObserver
+        https://bugs.webkit.org/show_bug.cgi?id=176364
+        <rdar://problem/34254780>
+
+        Reviewed by Alex Christensen.
+
+        * fast/events/beforeunload-dom-manipulation-crash-expected.txt: Added.
+        * fast/events/beforeunload-dom-manipulation-crash.html: Added.
+
 2017-09-14  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         fast/forms/append-children-during-form-submission.html fails due to a text diff on iOS

diff --git a/LayoutTests/fast/events/beforeunload-dom-manipulation-crash-expected.txt b/LayoutTests/fast/events/beforeunload-dom-manipulation-crash-expected.txt
@@ -0,0 +1 @@
+This test passes if it does not crash.
diff --git a/LayoutTests/fast/events/beforeunload-dom-manipulation-crash.html b/LayoutTests/fast/events/beforeunload-dom-manipulation-crash.html
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<head>
+<script src="../../resources/js-test.js"></script>
+<script>
+jsTestIsAsync = true;
+
+function runTest() {
+    window.onbeforeunload = nextStep;
+
+    iframe.name = "foo";
+    iframe.src = "data:text/html,foo";
+
+    location.href = "data:text/html,FAIL";
+}
+
+function nextStep() {
+    document.head.appendChild(del);
+    if (window.testRunner)
+        testRunner.forceImmediateCompletion();
+}
+</script>
+</head>
+<body onload="runTest()">
+    <p>This test passes if it does not crash.</p>
+    <del id="del">
+    <iframe id="iframe"></iframe>
+</body>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,26 @@
+2017-09-15  Brent Fulgham  <bfulgham@apple.com>
+
+        Make DocumentLoader a FrameDestructionObserver
+        https://bugs.webkit.org/show_bug.cgi?id=176364
+        <rdar://problem/34254780>
+
+        Reviewed by Alex Christensen.
+
+        The DocumentLoader needs to know when its Frame is destroyed so that it can
+        perform properly cleanup.
+
+        Test: fast/events/beforeunload-dom-manipulation-crash.html
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::DocumentLoader): Call FrameDestructionObserver constructor.
+        (WebCore::DocumentLoader::responseReceived): Drive-by fix. Make sure the current
+        object is valid during the callback.
+        (WebCore::DocumentLoader::attachToFrame): Use FrameDestructionObserver::observerFrame rather
+        than setting the m_frame variable directly.
+        (WebCore::DocumentLoader::detachFromFrame): Ditto.
+        * loader/DocumentLoader.h:
+        (WebCore::DocumentLoader::frame const): Deleted, as this is provided by the FrameDestructionObserver.
+
 2017-09-15  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [Harfbuzz] Material icons not rendered correctly when using the web font

diff --git a/Source/WebCore/loader/DocumentLoader.cpp b/Source/WebCore/loader/DocumentLoader.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2017 Apple Inc. All rights reserved.
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -133,7 +133,8 @@ static bool areAllLoadersPageCacheAcceptable(const ResourceLoaderMap& loaders)
 }
 
 DocumentLoader::DocumentLoader(const ResourceRequest& request, const SubstituteData& substituteData)
-    : m_cachedResourceLoader(CachedResourceLoader::create(this))
+    : FrameDestructionObserver(nullptr)
+    , m_cachedResourceLoader(CachedResourceLoader::create(this))
     , m_writer(m_frame)
     , m_originalRequest(request)
     , m_substituteData(substituteData)
@@ -704,7 +705,7 @@ void DocumentLoader::responseReceived(const ResourceResponse& response)
     }
 #endif
 
-    frameLoader()->checkContentPolicy(m_response, [this](PolicyAction policy) {
+    frameLoader()->checkContentPolicy(m_response, [this, protectedThis = makeRef(*this)](PolicyAction policy) {
         continueAfterContentPolicy(policy);
     });
 }
@@ -982,7 +983,7 @@ void DocumentLoader::attachToFrame(Frame& frame)
         return;
 
     ASSERT(!m_frame);
-    m_frame = &frame;
+    observeFrame(&frame);
     m_writer.setFrame(&frame);
     attachToFrame();
 
@@ -1023,7 +1024,7 @@ void DocumentLoader::detachFromFrame()
 
     InspectorInstrumentation::loaderDetachedFromFrame(*m_frame, *this);
 
-    m_frame = nullptr;
+    observeFrame(nullptr);
 }
 
 void DocumentLoader::clearMainResourceLoader()

diff --git a/Source/WebCore/loader/DocumentLoader.h b/Source/WebCore/loader/DocumentLoader.h
@@ -32,6 +32,7 @@
 #include "CachedRawResourceClient.h"
 #include "CachedResourceHandle.h"
 #include "DocumentWriter.h"
+#include "FrameDestructionObserver.h"
 #include "LinkIcon.h"
 #include "LoadTiming.h"
 #include "NavigationAction.h"
@@ -90,7 +91,7 @@ enum class AutoplayQuirk {
     InheritedUserGestures = 1 << 1,
 };
 
-class DocumentLoader : public RefCounted<DocumentLoader>, private CachedRawResourceClient {
+class DocumentLoader : public RefCounted<DocumentLoader>, public FrameDestructionObserver, private CachedRawResourceClient {
     WTF_MAKE_FAST_ALLOCATED;
     friend class ContentFilter;
 public:
@@ -101,7 +102,6 @@ class DocumentLoader : public RefCounted<DocumentLoader>, private CachedRawResou
     WEBCORE_EXPORT virtual ~DocumentLoader();
 
     void attachToFrame(Frame&);
-    Frame* frame() const { return m_frame; }
 
     WEBCORE_EXPORT virtual void detachFromFrame();
 
@@ -363,7 +363,6 @@ class DocumentLoader : public RefCounted<DocumentLoader>, private CachedRawResou
 
     void notifyFinishedLoadingIcon(uint64_t callbackIdentifier, SharedBuffer*);
 
-    Frame* m_frame { nullptr };
     Ref<CachedResourceLoader> m_cachedResourceLoader;
 
     CachedResourceHandle<CachedRawResource> m_mainResource;

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,27 @@
+2017-09-15  Brent Fulgham  <bfulgham@apple.com>
+
+        Provide mechanism to immediately end tests
+        https://bugs.webkit.org/show_bug.cgi?id=176364
+        <rdar://problem/34254780>
+
+        Reviewed by Alex Christensen.
+
+        WebKitTestRunner does not output state if the top loading frame has not been removed. This prevents some
+        tests that attempt to exercise failed load state from working properly.
+
+        This change adds a new 'forceImmediateCompletion' handler for DumpRenderTree and WebKitTestRunner so
+        that we can properly test these conditions.
+
+        * DumpRenderTree/TestRunner.h:
+        * DumpRenderTree/mac/TestRunnerMac.mm:
+        (TestRunner::forceImmediateCompletion): Added.
+        * DumpRenderTree/win/TestRunnerWin.cpp:
+        (TestRunner::forceImmediateCompletion): Ditto.
+        * WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
+        * WebKitTestRunner/InjectedBundle/TestRunner.cpp:
+        (WTR::TestRunner::forceImmediateCompletion): Ditto.
+        * WebKitTestRunner/InjectedBundle/TestRunner.h:
+
 2017-09-14  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [GTK] Add a switch to start the mini-browser in full-screen mode

diff --git a/Tools/DumpRenderTree/TestRunner.h b/Tools/DumpRenderTree/TestRunner.h
@@ -73,6 +73,7 @@ class TestRunner : public WTR::UIScriptContextDelegate, public RefCounted<TestRu
     void displayAndTrackRepaints();
     void execCommand(JSStringRef name, JSStringRef value);
     bool findString(JSContextRef, JSStringRef, JSObjectRef optionsArray);
+    void forceImmediateCompletion();
     void goBack();
     JSValueRef originsWithApplicationCache(JSContextRef);
     long long applicationCacheDiskUsageForOrigin(JSStringRef name);

diff --git a/Tools/DumpRenderTree/mac/TestRunnerMac.mm b/Tools/DumpRenderTree/mac/TestRunnerMac.mm
@@ -290,6 +290,13 @@ JSValueRef originsArrayToJS(JSContextRef context, NSArray *origins)
     m_waitToDump = false;
 }
 
+void TestRunner::forceImmediateCompletion()
+{
+    if (m_waitToDump && !WorkQueue::singleton().count())
+        dump();
+    m_waitToDump = false;
+}
+
 static inline std::string stringFromJSString(JSStringRef jsString)
 {
     size_t maxBufferSize = JSStringGetMaximumUTF8CStringSize(jsString);

diff --git a/Tools/DumpRenderTree/win/TestRunnerWin.cpp b/Tools/DumpRenderTree/win/TestRunnerWin.cpp
@@ -293,6 +293,14 @@ void TestRunner::notifyDone()
     m_waitToDump = false;
 }
 
+void TestRunner::forceImmediateCompletion()
+{
+    // Same as on mac. This can be shared.
+    if (m_waitToDump && !WorkQueue::singleton().count())
+        dump();
+    m_waitToDump = false;
+}
+
 static wstring jsStringRefToWString(JSStringRef jsStr)
 {
     size_t length = JSStringGetLength(jsStr);

diff --git a/Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl b/Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl
@@ -101,6 +101,9 @@ interface TestRunner {
     void display();
     void displayAndTrackRepaints();
 
+    // Failed load condition testing
+    void forceImmediateCompletion();
+
     // Printing
     boolean isPageBoxVisible(long pageIndex);
 

diff --git a/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp b/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
@@ -191,6 +191,18 @@ void TestRunner::notifyDone()
     m_waitToDump = false;
 }
 
+void TestRunner::forceImmediateCompletion()
+{
+    auto& injectedBundle = InjectedBundle::singleton();
+    if (!injectedBundle.isTestRunning())
+        return;
+
+    if (m_waitToDump && injectedBundle.page())
+        injectedBundle.page()->dump();
+
+    m_waitToDump = false;
+}
+
 unsigned TestRunner::imageCountInGeneralPasteboard() const
 {
     return InjectedBundle::singleton().imageCountInGeneralPasteboard();

diff --git a/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h b/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
@@ -164,6 +164,9 @@ class TestRunner : public JSWrappable {
     bool shouldDisallowIncreaseForApplicationCacheQuota() { return m_disallowIncreaseForApplicationCacheQuota; }
     JSValueRef originsWithApplicationCache();
 
+    // Failed load condition testing
+    void forceImmediateCompletion();
+
     // Printing
     bool isPageBoxVisible(int pageIndex);
     bool isPrinting() { return m_isPrinting; }