Changeset 86945 in webkit

Timestamp:

May 20, 2011 6:24:29 AM (13 years ago)

Author:

Adam Roben

Message:

Don't try to process DownloadProxy messages twice (and robustify code that runs if we do)

Fixes <​http://webkit.org/b/61142> <rdar://problem/9471680> REGRESSION (r86812): Crash
(preceded by assertion) in fastMalloc when downloading a file

Reviewed by Darin Adler.

Source/WebKit2:

• Platform/CoreIPC/ArgumentDecoder.cpp:

(CoreIPC::alignedBufferIsLargeEnoughToContain): Added. This helper function checks that the
given buffer is large enough to hold |size| bytes (and correctly handles the case where
we're already at the end or beyond the end of the buffer).

(CoreIPC::ArgumentDecoder::alignBufferPosition):
(CoreIPC::ArgumentDecoder::bufferIsLargeEnoughToContain):
Replaced old code that was vulnerable to underflow with the new helper function.

• UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::didReceiveSyncMessage): Added back an early return that was
mistakenly removed in r86812 so that we don't mistakenly pass DownloadProxy messages on to
a WebPageProxy after we've already handled them.

Tools:

Test that the WebKit2 UI process doesn't crash when starting a download

• TestWebKitAPI/Tests/WebKit2/18-characters.html: Added.

• TestWebKitAPI/Tests/WebKit2/DownloadDecideDestinationCrash.cpp: Added.

(TestWebKitAPI::decidePolicyForNavigationAction): Start a download.
(TestWebKitAPI::decideDestinationWithSuggestedFilename): Record that the download was
started, cancel the download, and return a bogus string.

(TestWebKitAPI::setContextDownloadClient):
(TestWebKitAPI::setPagePolicyClient):
Simple helper functions.

(TestWebKitAPI::TEST): Load 18-characters.html, which should trigger a download thanks to
our policy client, and run until we know that the download was started. If we haven't
crashed, we win!

• TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
• TestWebKitAPI/win/TestWebKitAPI.vcproj:
• TestWebKitAPI/win/copy-resources.cmd:

Added new files.

Location:

trunk

Files:

• Source/WebKit2/ChangeLog (modified) (1 diff)
• Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp (modified) (3 diffs)
• Source/WebKit2/UIProcess/WebProcessProxy.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj (modified) (5 diffs)
• Tools/TestWebKitAPI/Tests/WebKit2/18-characters.html (added)
• Tools/TestWebKitAPI/Tests/WebKit2/DownloadDecideDestinationCrash.cpp (added)
• Tools/TestWebKitAPI/win/TestWebKitAPI.vcproj (modified) (2 diffs)
• Tools/TestWebKitAPI/win/copy-resources.cmd (modified) (1 diff)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2011-05-19  Adam Roben  <aroben@apple.com>
+
+        Don't try to process DownloadProxy messages twice (and robustify code that runs if we do)
+
+        Fixes <http://webkit.org/b/61142> <rdar://problem/9471680> REGRESSION (r86812): Crash
+        (preceded by assertion) in fastMalloc when downloading a file
+
+        Reviewed by Darin Adler.
+
+        * Platform/CoreIPC/ArgumentDecoder.cpp:
+        (CoreIPC::alignedBufferIsLargeEnoughToContain): Added. This helper function checks that the
+        given buffer is large enough to hold |size| bytes (and correctly handles the case where
+        we're already at the end or beyond the end of the buffer).
+
+        (CoreIPC::ArgumentDecoder::alignBufferPosition):
+        (CoreIPC::ArgumentDecoder::bufferIsLargeEnoughToContain):
+        Replaced old code that was vulnerable to underflow with the new helper function.
+
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::didReceiveSyncMessage): Added back an early return that was
+        mistakenly removed in r86812 so that we don't mistakenly pass DownloadProxy messages on to
+        a WebPageProxy after we've already handled them.
+
 2011-05-19  Jer Noble  <jer.noble@apple.com>
 

trunk/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp

@@ -80 +80 @@
 }
 
+static inline bool alignedBufferIsLargeEnoughToContain(const uint8_t* alignedPosition, const uint8_t* bufferEnd, size_t size)
+{
+    return bufferEnd >= alignedPosition && static_cast<size_t>(bufferEnd - alignedPosition) >= size;
+}
+
 bool ArgumentDecoder::alignBufferPosition(unsigned alignment, size_t size)
 {
-    uint8_t* buffer = roundUpToAlignment(m_bufferPos, alignment);
-    if (static_cast<size_t>(m_bufferEnd - buffer) < size) {
+    uint8_t* alignedPosition = roundUpToAlignment(m_bufferPos, alignment);
+    if (!alignedBufferIsLargeEnoughToContain(alignedPosition, m_bufferEnd, size)) {
         // We've walked off the end of this buffer.
         markInvalid();
@@ -89 +94 @@
     }
     
-    m_bufferPos = buffer;
+    m_bufferPos = alignedPosition;
     return true;
 }
@@ -95 +100 @@
 bool ArgumentDecoder::bufferIsLargeEnoughToContain(unsigned alignment, size_t size) const
 {
-    return static_cast<size_t>(m_bufferEnd - roundUpToAlignment(m_bufferPos, alignment)) >= size;
+    return alignedBufferIsLargeEnoughToContain(roundUpToAlignment(m_bufferPos, alignment), m_bufferEnd, size);
 }
 

trunk/Source/WebKit2/UIProcess/WebProcessProxy.cpp

@@ -271 +271 @@
         || messageID.is<CoreIPC::MessageClassDownloadProxy>() || messageID.is<CoreIPC::MessageClassWebIconDatabase>()) {
         m_context->didReceiveSyncMessage(connection, messageID, arguments, reply);
+        return;
     }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2011-05-19  Adam Roben  <aroben@apple.com>
+
+        Test that the WebKit2 UI process doesn't crash when starting a download
+
+        Test for <http://webkit.org/b/61142> <rdar://problem/9471680> REGRESSION (r86812): Crash
+        (preceded by assertion) in fastMalloc when downloading a file
+
+        Reviewed by Darin Adler.
+
+        * TestWebKitAPI/Tests/WebKit2/18-characters.html: Added.
+
+        * TestWebKitAPI/Tests/WebKit2/DownloadDecideDestinationCrash.cpp: Added.
+        (TestWebKitAPI::decidePolicyForNavigationAction): Start a download.
+        (TestWebKitAPI::decideDestinationWithSuggestedFilename): Record that the download was
+        started, cancel the download, and return a bogus string.
+
+        (TestWebKitAPI::setContextDownloadClient):
+        (TestWebKitAPI::setPagePolicyClient):
+        Simple helper functions.
+
+        (TestWebKitAPI::TEST): Load 18-characters.html, which should trigger a download thanks to
+        our policy client, and run until we know that the download was started. If we haven't
+        crashed, we win!
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/win/TestWebKitAPI.vcproj:
+        * TestWebKitAPI/win/copy-resources.cmd:
+        Added new files.
+
 2011-05-20  Kent Tamura  <tkent@chromium.org>
 

trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

@@ -56 +56 @@
                 C02B77F2126612140026BF0F /* SpacebarScrolling.cpp in Sources */ = {isa = PBXBuildFile; fileRef = C02B77F1126612140026BF0F /* SpacebarScrolling.cpp */; };
                 C02B7854126613AE0026BF0F /* Carbon.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = C02B7853126613AE0026BF0F /* Carbon.framework */; };
+                C045F9451385C2EA00C0F3CD /* DownloadDecideDestinationCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = C045F9441385C2E900C0F3CD /* DownloadDecideDestinationCrash.cpp */; };
                 C0ADBE7C12FCA4D000D2C129 /* JavaScriptTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = C0ADBE7A12FCA4D000D2C129 /* JavaScriptTest.cpp */; };
                 C0ADBE8312FCA6AA00D2C129 /* RestoreSessionStateContainingFormData.cpp in Sources */ = {isa = PBXBuildFile; fileRef = C0ADBE8212FCA6AA00D2C129 /* RestoreSessionStateContainingFormData.cpp */; };
@@ -164 +165 @@
                 C02B7853126613AE0026BF0F /* Carbon.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; name = Carbon.framework; sourceTree = SDKROOT; };
                 C02B7882126615410026BF0F /* spacebar-scrolling.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "spacebar-scrolling.html"; sourceTree = "<group>"; };
+                C045F9441385C2E900C0F3CD /* DownloadDecideDestinationCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = DownloadDecideDestinationCrash.cpp; sourceTree = "<group>"; };
+                C045F9461385C2F800C0F3CD /* 18-characters.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "18-characters.html"; sourceTree = "<group>"; };
                 C0ADBE7A12FCA4D000D2C129 /* JavaScriptTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JavaScriptTest.cpp; sourceTree = "<group>"; };
                 C0ADBE7B12FCA4D000D2C129 /* JavaScriptTest.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JavaScriptTest.h; sourceTree = "<group>"; };
@@ -285 +288 @@
                                 BCB6803F126FBFE100642A61 /* DocumentStartUserScriptAlertCrash.cpp */,
                                 BCB68041126FBFF100642A61 /* DocumentStartUserScriptAlertCrash_Bundle.cpp */,
+                                C045F9441385C2E900C0F3CD /* DownloadDecideDestinationCrash.cpp */,
                                 1A5FEFDC1270E2A3000E2921 /* EvaluateJavaScript.cpp */,
                                 BCC8B95A12611F4700DE46A4 /* FailedLoad.cpp */,
@@ -322 +326 @@
                         isa = PBXGroup;
                         children = (
+                                C045F9461385C2F800C0F3CD /* 18-characters.html */,
                                 BC2D004A12A9FEB300E732A3 /* file-with-anchor.html */,
                                 1A02C84B125D4A5E00E3F4BD /* find.html */,
@@ -462 +467 @@
                                 F6F3F29113342FEB00A6BF19 /* CookieManager.cpp in Sources */,
                                 33BE5AF5137B5A6C00705813 /* MouseMoveAfterCrash.cpp in Sources */,
+                                C045F9451385C2EA00C0F3CD /* DownloadDecideDestinationCrash.cpp in Sources */,
                         );
                         runOnlyForDeploymentPostprocessing = 0;

trunk/Tools/TestWebKitAPI/win/TestWebKitAPI.vcproj

@@ -413 +413 @@
                                 >
                                 <File
+                                        RelativePath="..\Tests\WebKit2\18-characters.html"
+                                        >
+                                </File>
+                                <File
                                         RelativePath="..\Tests\WebKit2\AboutBlankLoad.cpp"
                                         >
@@ -426 +430 @@
                                 <File
                                         RelativePath="..\Tests\WebKit2\DocumentStartUserScriptAlertCrash.cpp"
+                                        >
+                                </File>
+                                <File
+                                        RelativePath="..\Tests\WebKit2\DownloadDecideDestinationCrash.cpp"
                                         >
                                 </File>

trunk/Tools/TestWebKitAPI/win/copy-resources.cmd

@@ -9 +9 @@
 mkdir 2>NUL "%ResourcesDirectory%"
 for %%f in (
+    ..\Tests\WebKit2\18-characters.html
     ..\Tests\WebKit2\file-with-anchor.html
     ..\Tests\WebKit2\find.html