Double release of resources if the load is canceled in a callback of ResourceLoader...

https://bugs.webkit.org/show_bug.cgi?id=90431

Patch by Benjamin Poulain <bpoulain@apple.com> on 2012-07-05
Reviewed by Anders Carlsson.

Source/WebCore:

In ResourceLoader::didFinishLoadingOnePart(), we invoke didFinishLoad() on the WebKit client. If WebKit
causes the current frame to cancel the load synchronously, the resources are already freed when
ResourceLoader::didFinishLoadingOnePart() ends.
When ResourceLoader::didFinishLoading() subsequently invokes releaseResources(), we are releasing the
resources a second time.

This patch add a second check for cancellation after invoking ResourceLoader::didFinishLoadingOnePart() to
avoid such issues.

The previous check at the beginning of ResourceLoader::didFinishLoading() has been removed because it is
redundant with ResourceLoader::didFinishLoadingOnePart().

* loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::didFinishLoading):
(WebCore::ResourceLoader::didFinishLoadingOnePart):

Tools:

Add a Mac API test.

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.html: Added.
* TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.mm: Added.
(-[CancelLoadFromResourceLoadDelegate webView:resource:didFinishLoadingFromDataSource:]):
(-[CancelLoadFromResourceLoadDelegateFrameLoadDelegate webView:didFinishLoadForFrame:]):
(TestWebKitAPI):
(TestWebKitAPI::TEST):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 8b0c0cc..6e38cc9 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,26 @@
+2012-07-05  Benjamin Poulain  <bpoulain@apple.com>
+
+        Double release of resources if the load is canceled in a callback of ResourceLoader::didFinishLoading
+        https://bugs.webkit.org/show_bug.cgi?id=90431
+
+        Reviewed by Anders Carlsson.
+
+        In ResourceLoader::didFinishLoadingOnePart(), we invoke didFinishLoad() on the WebKit client. If WebKit
+        causes the current frame to cancel the load synchronously, the resources are already freed when
+        ResourceLoader::didFinishLoadingOnePart() ends.
+        When ResourceLoader::didFinishLoading() subsequently invokes releaseResources(), we are releasing the
+        resources a second time.
+
+        This patch add a second check for cancellation after invoking ResourceLoader::didFinishLoadingOnePart() to
+        avoid such issues.
+
+        The previous check at the beginning of ResourceLoader::didFinishLoading() has been removed because it is
+        redundant with ResourceLoader::didFinishLoadingOnePart().
+
+        * loader/ResourceLoader.cpp:
+        (WebCore::ResourceLoader::didFinishLoading):
+        (WebCore::ResourceLoader::didFinishLoadingOnePart):
+
 2012-07-05  Simon Fraser  <simon.fraser@apple.com>
 
         Add a utility method for hasOverflowClip() or hasClip()

diff --git a/Source/WebCore/loader/ResourceLoader.cpp b/Source/WebCore/loader/ResourceLoader.cpp
index 5a064d0..3da77bc 100644 (file)
--- a/Source/WebCore/loader/ResourceLoader.cpp
+++ b/Source/WebCore/loader/ResourceLoader.cpp
@@ -288,18 +288,19 @@ void ResourceLoader::willStopBufferingData(const char* data, int length)
 
 void ResourceLoader::didFinishLoading(double finishTime)
 {
-    // If load has been cancelled after finishing (which could happen with a 
-    // JavaScript that changes the window location), do nothing.
+    didFinishLoadingOnePart(finishTime);
+
+    // If the load has been cancelled by a delegate in response to didFinishLoad(), do not release
+    // the resources a second time, they have been released by cancel.
     if (m_cancelled)
         return;
-    ASSERT(!m_reachedTerminalState);
-
-    didFinishLoadingOnePart(finishTime);
     releaseResources();
 }
 
 void ResourceLoader::didFinishLoadingOnePart(double finishTime)
 {
+    // If load has been cancelled after finishing (which could happen with a
+    // JavaScript that changes the window location), do nothing.
     if (m_cancelled)
         return;
     ASSERT(!m_reachedTerminalState);

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
index 49e987a..363b34b 100644 (file)
--- a/Tools/ChangeLog
+++ b/Tools/ChangeLog
@@ -1,3 +1,20 @@
+2012-07-05  Benjamin Poulain  <bpoulain@apple.com>
+
+        Double release of resources if the load is canceled in a callback of ResourceLoader::didFinishLoading
+        https://bugs.webkit.org/show_bug.cgi?id=90431
+
+        Reviewed by Anders Carlsson.
+
+        Add a Mac API test.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.html: Added.
+        * TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.mm: Added.
+        (-[CancelLoadFromResourceLoadDelegate webView:resource:didFinishLoadingFromDataSource:]):
+        (-[CancelLoadFromResourceLoadDelegateFrameLoadDelegate webView:didFinishLoadForFrame:]):
+        (TestWebKitAPI):
+        (TestWebKitAPI::TEST):
+
 2012-07-05  Dave Tharp  <dtharp@codeaurora.org>
 
         Adding myself as committer to committers.py

diff --git a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
index f75a207..9134a93 100644 (file)
--- a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
+++ b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
@@ -19,6 +19,8 @@
                1ADBEFAE130C689C00D61D19 /* ForceRepaint.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 1ADBEFAD130C689C00D61D19 /* ForceRepaint.cpp */; };
                1ADBEFE3130C6AA100D61D19 /* simple-accelerated-compositing.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 1ADBEFBC130C6A0100D61D19 /* simple-accelerated-compositing.html */; };
                1AEDE22613E5E7E700E62FE8 /* InjectedBundleControllerMac.mm in Sources */ = {isa = PBXBuildFile; fileRef = 1AEDE22413E5E7A000E62FE8 /* InjectedBundleControllerMac.mm */; };
+               26DF5A5E15A29BAA003689C2 /* CancelLoadFromResourceLoadDelegate.mm in Sources */ = {isa = PBXBuildFile; fileRef = 26DF5A5D15A29BAA003689C2 /* CancelLoadFromResourceLoadDelegate.mm */; };
+               26DF5A6315A2A27E003689C2 /* CancelLoadFromResourceLoadDelegate.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 26DF5A6115A2A22B003689C2 /* CancelLoadFromResourceLoadDelegate.html */; };
                333B9CE21277F23100FEFCE3 /* PreventEmptyUserAgent.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 333B9CE11277F23100FEFCE3 /* PreventEmptyUserAgent.cpp */; };
                33BE5AF5137B5A6C00705813 /* MouseMoveAfterCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 33BE5AF4137B5A6C00705813 /* MouseMoveAfterCrash.cpp */; };
                33BE5AF9137B5AAE00705813 /* MouseMoveAfterCrash_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 33BE5AF8137B5AAE00705813 /* MouseMoveAfterCrash_Bundle.cpp */; };
@@ -189,6 +191,7 @@
                                B55F11BE15191A0600915916 /* Ahem.ttf in Copy Resources */,
                                B55F11B71517D03300915916 /* attributedStringCustomFont.html in Copy Resources */,
                                76E182DF154767E600F1FADD /* auto-submitting-form.html in Copy Resources */,
+                               26DF5A6315A2A27E003689C2 /* CancelLoadFromResourceLoadDelegate.html in Copy Resources */,
                                5142B2731517C8C800C32B19 /* ContextMenuCanCopyURL.html in Copy Resources */,
                                9B4F8FA7159D52DD002D9F94 /* HTMLCollectionNamedItem.html in Copy Resources */,
                                9B26FCCA159D16DE00CC3765 /* HTMLFormCollectionNamedItem.html in Copy Resources */,
@@ -231,6 +234,8 @@
                1ADBEFAD130C689C00D61D19 /* ForceRepaint.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ForceRepaint.cpp; sourceTree = "<group>"; };
                1ADBEFBC130C6A0100D61D19 /* simple-accelerated-compositing.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "simple-accelerated-compositing.html"; sourceTree = "<group>"; };
                1AEDE22413E5E7A000E62FE8 /* InjectedBundleControllerMac.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = InjectedBundleControllerMac.mm; sourceTree = "<group>"; };
+               26DF5A5D15A29BAA003689C2 /* CancelLoadFromResourceLoadDelegate.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = CancelLoadFromResourceLoadDelegate.mm; sourceTree = "<group>"; };
+               26DF5A6115A2A22B003689C2 /* CancelLoadFromResourceLoadDelegate.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = CancelLoadFromResourceLoadDelegate.html; sourceTree = "<group>"; };
                333B9CE11277F23100FEFCE3 /* PreventEmptyUserAgent.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = PreventEmptyUserAgent.cpp; sourceTree = "<group>"; };
                33BE5AF4137B5A6C00705813 /* MouseMoveAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = MouseMoveAfterCrash.cpp; sourceTree = "<group>"; };
                33BE5AF8137B5AAE00705813 /* MouseMoveAfterCrash_Bundle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = MouseMoveAfterCrash_Bundle.cpp; sourceTree = "<group>"; };
@@ -663,6 +668,7 @@
                                C07E6CB013FD737C0038B22B /* Resources */,
                                379028B514FABD92007E6B43 /* AcceptsFirstMouse.mm */,
                                B55F119F1516834F00915916 /* AttributedString.mm */,
+                               26DF5A5D15A29BAA003689C2 /* CancelLoadFromResourceLoadDelegate.mm */,
                                5142B2701517C88B00C32B19 /* ContextMenuCanCopyURL.mm */,
                                37DC678B140D7C5000ABCCDB /* DOMRangeOfString.mm */,
                                C07E6CAE13FD67650038B22B /* DynamicDeviceScaleFactor.mm */,
@@ -691,6 +697,7 @@
                                B55F11B9151916E600915916 /* Ahem.ttf */,
                                B55F11B01517A2C400915916 /* attributedStringCustomFont.html */,
                                379028B814FABE49007E6B43 /* acceptsFirstMouse.html */,
+                               26DF5A6115A2A22B003689C2 /* CancelLoadFromResourceLoadDelegate.html */,
                                5142B2721517C89100C32B19 /* ContextMenuCanCopyURL.html */,
                                37DC678F140D7D3A00ABCCDB /* DOMRangeOfString.html */,
                                9B4F8FA6159D52CA002D9F94 /* HTMLCollectionNamedItem.html */,
@@ -907,6 +914,7 @@
                                52B8CF9615868CF000281053 /* SetDocumentURI.mm in Sources */,
                                9B26FC6C159D061000CC3765 /* HTMLFormCollectionNamedItem.mm in Sources */,
                                9B4F8FA4159D52B1002D9F94 /* HTMLCollectionNamedItem.mm in Sources */,
+                               26DF5A5E15A29BAA003689C2 /* CancelLoadFromResourceLoadDelegate.mm in Sources */,
                        );
                        runOnlyForDeploymentPostprocessing = 0;
                };

diff --git a/Tools/TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.html b/Tools/TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.html
new file mode 100644 (file)
index 0000000..efa4d1e
--- /dev/null
+++ b/Tools/TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.html
@@ -0,0 +1,8 @@
+<script>
+    function cancelLoadAndJumpToBlank() {
+        window.stop();
+        window.location = "about:blank";
+    }
+</script>
+<img src="Ahem.ttf">
+<script src="about:blank"></script>
\ No newline at end of file

diff --git a/Tools/TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.mm b/Tools/TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.mm
new file mode 100644 (file)
index 0000000..3729eb5
--- /dev/null
+++ b/Tools/TestWebKitAPI/Tests/mac/CancelLoadFromResourceLoadDelegate.mm
@@ -0,0 +1,82 @@
+/*
+ * Copyright (C) 2012 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#import "config.h"
+#import "PlatformUtilities.h"
+#import <wtf/RetainPtr.h>
+
+@interface CancelLoadFromResourceLoadDelegate : NSObject {
+    size_t resourceCount;
+}
+
+@end
+
+@implementation CancelLoadFromResourceLoadDelegate
+
+- (void)webView:(WebView *)sender resource:(id)identifier didFinishLoadingFromDataSource:(WebDataSource *)dataSource
+{
+    // Break the load once we have loaded the <script> and <img>.
+    ++resourceCount;
+    if (resourceCount > 2)
+        [sender stringByEvaluatingJavaScriptFromString:@"cancelLoadAndJumpToBlank()"];
+}
+@end
+
+
+static bool didFinishLoad = false;
+
+@interface CancelLoadFromResourceLoadDelegateFrameLoadDelegate : NSObject
+@end
+
+@implementation CancelLoadFromResourceLoadDelegateFrameLoadDelegate
+- (void)webView:(WebView *)sender didFinishLoadForFrame:(WebFrame *)frame
+{
+    if ([[sender mainFrameURL] isEqualToString:@"about:blank"])
+        didFinishLoad = true;
+}
+@end
+
+namespace TestWebKitAPI {
+
+TEST(WebKit1, CancelLoadFromResourceLoadDelegate)
+{
+    NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
+
+    RetainPtr<WebView> webView(AdoptNS, [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 120, 200) frameName:nil groupName:nil]);
+
+    RetainPtr<CancelLoadFromResourceLoadDelegate> resourceLoadDelegate(AdoptNS, [[CancelLoadFromResourceLoadDelegate alloc] init]);
+    webView.get().resourceLoadDelegate = resourceLoadDelegate.get();
+    RetainPtr<CancelLoadFromResourceLoadDelegateFrameLoadDelegate> frameLoadDelegate(AdoptNS, [[CancelLoadFromResourceLoadDelegateFrameLoadDelegate alloc] init]);
+    webView.get().frameLoadDelegate = frameLoadDelegate.get();
+
+    [[webView.get() mainFrame] loadRequest:[NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"CancelLoadFromResourceLoadDelegate" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]]];
+
+    Util::run(&didFinishLoad);
+
+    [pool drain];
+    // If we finished without crashing, the test passed.
+}
+
+} // namespace TestWebKitAPI