Handle a null FindIndicator correctly
authoraroben@apple.com <aroben@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Feb 2011 19:26:07 +0000 (19:26 +0000)
committeraroben@apple.com <aroben@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Feb 2011 19:26:07 +0000 (19:26 +0000)
We are passed a null FindIndicator when the find indicator becomes hidden.

Fixes <http://webkit.org/b/54213> <rdar://problem/8983261> REGRESSION (r78198): Crash in
FindIndicator::contentImage when scrolling page

Reviewed by Steve Falkenburg.

Source/WebKit2:

* Platform/win/SharedMemoryWin.cpp:
(WebKit::SharedMemory::Handle::isNull): Implemented.
(WebKit::SharedMemory::create): Bail out if the handle is null, just like Mac does.
Otherwise assert that ::MapViewOfFile succeeded. This doesn't fix the bug, but should help
catch other errors.

* UIProcess/win/WebView.cpp:
(WebKit::WebView::setFindIndicator): Null-check the FindIndicator before dereferencing it.
Also changed the function to store the FindIndicator in a RefPtr.

Tools:

Test showing and hiding the find indicator on Windows

* TestWebKitAPI/Tests/WebKit2/win/HideFindIndicator.cpp: Added.
(TestWebKitAPI::didFinishLoadForFrame): Record that the load finished.
(TestWebKitAPI::findIndicatorCallback): Record that the callback was called, and save the
bitmap.
(TestWebKitAPI::initialize): Hook up our callbacks.
(TestWebKitAPI::TEST): Test showing then hiding the find indicator to see if we crash.

* TestWebKitAPI/PlatformWebView.h:
* TestWebKitAPI/mac/PlatformWebViewMac.mm:
(TestWebKitAPI::PlatformWebView::page):
* TestWebKitAPI/win/PlatformWebViewWin.cpp:
(TestWebKitAPI::PlatformWebView::page):
Made page a const member function.

* TestWebKitAPI/win/TestWebKitAPI.vcproj: Added the new test.

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

Source/WebKit2/ChangeLog
Source/WebKit2/Platform/win/SharedMemoryWin.cpp
Source/WebKit2/UIProcess/win/WebView.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/PlatformWebView.h
Tools/TestWebKitAPI/Tests/WebKit2/win/HideFindIndicator.cpp [new file with mode: 0644]
Tools/TestWebKitAPI/mac/PlatformWebViewMac.mm
Tools/TestWebKitAPI/win/PlatformWebViewWin.cpp
Tools/TestWebKitAPI/win/TestWebKitAPI.vcproj

index 3cd7113..6d810b9 100644 (file)
@@ -1,3 +1,24 @@
+2011-02-10  Adam Roben  <aroben@apple.com>
+
+        Handle a null FindIndicator correctly
+
+        We are passed a null FindIndicator when the find indicator becomes hidden.
+
+        Fixes <http://webkit.org/b/54213> <rdar://problem/8983261> REGRESSION (r78198): Crash in
+        FindIndicator::contentImage when scrolling page
+
+        Reviewed by Steve Falkenburg.
+
+        * Platform/win/SharedMemoryWin.cpp:
+        (WebKit::SharedMemory::Handle::isNull): Implemented.
+        (WebKit::SharedMemory::create): Bail out if the handle is null, just like Mac does.
+        Otherwise assert that ::MapViewOfFile succeeded. This doesn't fix the bug, but should help
+        catch other errors.
+
+        * UIProcess/win/WebView.cpp:
+        (WebKit::WebView::setFindIndicator): Null-check the FindIndicator before dereferencing it.
+        Also changed the function to store the FindIndicator in a RefPtr.
+
 2011-02-10  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by Sam Weinig.
index 1381c7f..ef83de7 100644 (file)
@@ -46,6 +46,11 @@ SharedMemory::Handle::~Handle()
     ::CloseHandle(m_handle);
 }
 
+bool SharedMemory::Handle::isNull() const
+{
+    return !m_handle;
+}
+
 void SharedMemory::Handle::encode(CoreIPC::ArgumentEncoder* encoder) const
 {
     encoder->encodeUInt64(m_size);
@@ -141,9 +146,13 @@ static DWORD accessRights(SharedMemory::Protection protection)
 
 PassRefPtr<SharedMemory> SharedMemory::create(const Handle& handle, Protection protection)
 {
+    if (handle.isNull())
+        return 0;
+
     DWORD desiredAccess = accessRights(protection);
 
     void* baseAddress = ::MapViewOfFile(handle.m_handle, desiredAccess, 0, 0, handle.m_size);
+    ASSERT_WITH_MESSAGE(baseAddress, "::MapViewOfFile failed with error %lu", ::GetLastError());
     if (!baseAddress)
         return 0;
 
index 3b5726e..eea43c5 100644 (file)
@@ -1060,35 +1060,37 @@ PassRefPtr<WebContextMenuProxy> WebView::createContextMenuProxy(WebPageProxy* pa
     return WebContextMenuProxyWin::create(m_window, page);
 }
 
-void WebView::setFindIndicator(PassRefPtr<FindIndicator> findIndicator, bool fadeOut)
+void WebView::setFindIndicator(PassRefPtr<FindIndicator> prpFindIndicator, bool fadeOut)
 {
     if (!m_findIndicatorCallback)
         return;
-    
+
     HBITMAP hbmp = 0;
-    ShareableBitmap* contentImage = findIndicator->contentImage();
-    
-    if (contentImage) {
-        // Render the contentImage to an HBITMAP.
-        void* bits;
-        HDC hdc = ::CreateCompatibleDC(0);
-        int width = contentImage->bounds().width();
-        int height = contentImage->bounds().height();
-        BitmapInfo bitmapInfo = BitmapInfo::create(contentImage->size());
-
-        hbmp = CreateDIBSection(0, &bitmapInfo, DIB_RGB_COLORS, static_cast<void**>(&bits), 0, 0);
-        HBITMAP hbmpOld = static_cast<HBITMAP>(SelectObject(hdc, hbmp));
-        RetainPtr<CGContextRef> context(AdoptCF, CGBitmapContextCreate(bits, width, height,
-            8, width * sizeof(RGBQUAD), deviceRGBColorSpaceRef(), kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst));
-
-        GraphicsContext graphicsContext(context.get());
-        contentImage->paint(graphicsContext, IntPoint(), contentImage->bounds());
-
-        ::SelectObject(hdc, hbmpOld);
-        ::DeleteDC(hdc);
-    }
+    IntRect selectionRect;
+
+    if (RefPtr<FindIndicator> findIndicator = prpFindIndicator) {
+        if (ShareableBitmap* contentImage = findIndicator->contentImage()) {
+            // Render the contentImage to an HBITMAP.
+            void* bits;
+            HDC hdc = ::CreateCompatibleDC(0);
+            int width = contentImage->bounds().width();
+            int height = contentImage->bounds().height();
+            BitmapInfo bitmapInfo = BitmapInfo::create(contentImage->size());
+
+            hbmp = CreateDIBSection(0, &bitmapInfo, DIB_RGB_COLORS, static_cast<void**>(&bits), 0, 0);
+            HBITMAP hbmpOld = static_cast<HBITMAP>(SelectObject(hdc, hbmp));
+            RetainPtr<CGContextRef> context(AdoptCF, CGBitmapContextCreate(bits, width, height,
+                8, width * sizeof(RGBQUAD), deviceRGBColorSpaceRef(), kCGBitmapByteOrder32Little | kCGImageAlphaPremultipliedFirst));
+
+            GraphicsContext graphicsContext(context.get());
+            contentImage->paint(graphicsContext, IntPoint(), contentImage->bounds());
+
+            ::SelectObject(hdc, hbmpOld);
+            ::DeleteDC(hdc);
+        }
 
-    IntRect selectionRect(findIndicator->selectionRectInWindowCoordinates());
+        selectionRect = IntRect(findIndicator->selectionRectInWindowCoordinates());
+    }
     
     // The callback is responsible for calling ::DeleteObject(hbmp).
     (*m_findIndicatorCallback)(toAPI(this), hbmp, selectionRect, fadeOut, m_findIndicatorCallbackContext);
index ede458f..903aa08 100644 (file)
@@ -1,3 +1,28 @@
+2011-02-10  Adam Roben  <aroben@apple.com>
+
+        Test showing and hiding the find indicator on Windows
+
+        Test for <http://webkit.org/b/54213> <rdar://problem/8983261> REGRESSION (r78198): Crash in
+        FindIndicator::contentImage when scrolling page
+
+        Reviewed by Steve Falkenburg.
+
+        * TestWebKitAPI/Tests/WebKit2/win/HideFindIndicator.cpp: Added.
+        (TestWebKitAPI::didFinishLoadForFrame): Record that the load finished.
+        (TestWebKitAPI::findIndicatorCallback): Record that the callback was called, and save the
+        bitmap.
+        (TestWebKitAPI::initialize): Hook up our callbacks.
+        (TestWebKitAPI::TEST): Test showing then hiding the find indicator to see if we crash.
+
+        * TestWebKitAPI/PlatformWebView.h:
+        * TestWebKitAPI/mac/PlatformWebViewMac.mm:
+        (TestWebKitAPI::PlatformWebView::page):
+        * TestWebKitAPI/win/PlatformWebViewWin.cpp:
+        (TestWebKitAPI::PlatformWebView::page):
+        Made page a const member function.
+
+        * TestWebKitAPI/win/TestWebKitAPI.vcproj: Added the new test.
+
 2011-02-10  Mario Sanchez Prada  <msanchez@igalia.com>
 
         Reviewed by Martin Robinson.
index 43e329b..312168d 100644 (file)
@@ -54,7 +54,7 @@ public:
     PlatformWebView(WKContextRef, WKPageGroupRef = 0);
     ~PlatformWebView();
 
-    WKPageRef page();
+    WKPageRef page() const;
     PlatformWKView platformView() const { return m_view; }
     void resizeTo(unsigned width, unsigned height);
     void focus();
diff --git a/Tools/TestWebKitAPI/Tests/WebKit2/win/HideFindIndicator.cpp b/Tools/TestWebKitAPI/Tests/WebKit2/win/HideFindIndicator.cpp
new file mode 100644 (file)
index 0000000..40d4f41
--- /dev/null
@@ -0,0 +1,85 @@
+/*
+ * Copyright (C) 2010 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.
+ */
+
+#include "Test.h"
+
+#include "PlatformUtilities.h"
+#include "PlatformWebView.h"
+
+namespace TestWebKitAPI {
+
+static bool didFinishLoad;
+static bool findIndicatorCallbackWasCalled;
+static HBITMAP bitmap;
+
+static void didFinishLoadForFrame(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void* clientInfo)
+{
+    didFinishLoad = true;
+}
+
+static void findIndicatorCallback(WKViewRef, HBITMAP selectionBitmap, RECT, bool, void*)
+{
+    findIndicatorCallbackWasCalled = true;
+    bitmap = selectionBitmap;
+}
+
+static void initialize(const PlatformWebView& webView)
+{
+    WKPageLoaderClient loaderClient;
+    memset(&loaderClient, 0, sizeof(loaderClient));
+
+    loaderClient.version = 0;
+    loaderClient.didFinishLoadForFrame = didFinishLoadForFrame;
+    WKPageSetPageLoaderClient(webView.page(), &loaderClient);
+
+    WKViewSetFindIndicatorCallback(webView.platformView(), findIndicatorCallback, 0);
+}
+
+TEST(WebKit2, HideFindIndicator)
+{
+    WKRetainPtr<WKContextRef> context(AdoptWK, WKContextCreate());
+    PlatformWebView webView(context.get());
+    initialize(webView);
+
+    WKRetainPtr<WKURLRef> url = Util::adoptWK(Util::createURLForResource("find", "html"));
+    WKPageLoadURL(webView.page(), url.get());
+    Util::run(&didFinishLoad);
+    didFinishLoad = false;
+
+    WKPageFindString(webView.page(), Util::toWK("Hello").get(), kWKFindOptionsShowFindIndicator, 100);
+    Util::run(&findIndicatorCallbackWasCalled);
+    findIndicatorCallbackWasCalled = false;
+
+    TEST_ASSERT(bitmap);
+    ::DeleteObject(bitmap);
+    bitmap = 0;
+
+    WKPageHideFindUI(webView.page());
+    Util::run(&findIndicatorCallbackWasCalled);
+
+    TEST_ASSERT(!bitmap);
+}
+
+} // namespace TestWebKitAPI
index c4f2d72..ad901d3 100644 (file)
@@ -55,7 +55,7 @@ PlatformWebView::~PlatformWebView()
     [m_view release];
 }
 
-WKPageRef PlatformWebView::page()
+WKPageRef PlatformWebView::page() const
 {
     return [m_view pageRef];
 }
index 18367cb..01a76eb 100644 (file)
@@ -72,7 +72,7 @@ PlatformWebView::~PlatformWebView()
     WKRelease(m_view);
 }
 
-WKPageRef PlatformWebView::page()
+WKPageRef PlatformWebView::page() const
 {
     return WKViewGetPage(m_view);
 }
index 8a60f15..0c806a8 100644 (file)
                                                >
                                        </File>
                                        <File
+                                               RelativePath="..\Tests\WebKit2\win\HideFindIndicator.cpp"
+                                               >
+                                       </File>
+                                       <File
                                                RelativePath="..\Tests\WebKit2\win\ResizeViewWhileHidden.cpp"
                                                >
                                        </File>