Bug 172209 – [Win] An assertion fails if the custom cursor image isn't loaded yet

• Component: UI Events
• Status: RESOLVED FIXED

[Fujii Hironori]

I tested with WinCairo port, debug build, trunk@216964.
I see the following assertion failure while moving cursor in Google maps.

> ASSERTION FAILED: bmp
> C:\webkit\ga\Source\WebCore\platform\graphics\win\ImageCairoWin.cpp(60) : WebCore::BitmapImage::getHBITMAPOfSize

Callstack:

> WTF.dll!WTFCrash() Line 292	C++
> WebKit.dll!WebCore::BitmapImage::getHBITMAPOfSize(HBITMAP__ * bmp, const WebCore::IntSize * size) Line 60	C++
> WebKit.dll!WebCore::BitmapImage::getHBITMAP(HBITMAP__ * bmp) Line 52	C++
> WebKit.dll!WebCore::createSharedCursor(WebCore::Image * img, const WebCore::IntPoint & hotSpot) Line 70	C++
> WebKit.dll!WebCore::Cursor::ensurePlatformCursor() Line 260	C++
> WebKit.dll!WebCore::Cursor::platformCursor() Line 186	C++
> WebKit.dll!WebChromeClient::setCursor(const WebCore::Cursor & cursor) Line 693	C++
> WebKit.dll!WebCore::Chrome::setCursor(const WebCore::Cursor & cursor) Line 466	C++
> WebKit.dll!WebCore::Widget::setCursor(const WebCore::Cursor & cursor) Line 76	C++
> WebKit.dll!WebCore::EventHandler::updateCursor(WebCore::FrameView & view, const WebCore::HitTestResult & result, bool shiftKey) Line 1374	C++
> WebKit.dll!WebCore::EventHandler::handleMouseMoveEvent(const WebCore::PlatformMouseEvent & platformMouseEvent, WebCore::HitTestResult * hoveredNode, bool onlyUpdateScrollbars) Line 1974	C++
> WebKit.dll!WebCore::EventHandler::mouseMoved(const WebCore::PlatformMouseEvent & event) Line 1838	C++
> WebKit.dll!WebView::handleMouseEvent(unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 1884	C++
> WebKit.dll!WebView::WebViewWndProc(HWND__ * hWnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 2607	C++
> [External Code]	
> WebKit.dll!WebCore::WindowMessageBroadcaster::SubclassedWndProc(HWND__ * hwnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 125	C++
> [External Code]	
> WebKit.dll!WebKitMessageLoop::run(HACCEL__ * hAccelTable) Line 97	C++
> MiniBrowserLib.dll!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 189	C++
> MiniBrowserLib.dll!dllLauncherEntryPoint(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 857	C++
> MiniBrowser.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 249	C++
> [External Code]	

The cursor image wasn't loaded yet at the time.
EventHandler::selectCursor returns nullImage in such case.

There are two choices of solution:

  1. EventHandler::selectCursor should not select a custom cursor its image is not loaded yet
  2. Windows port should handle nullImage cursor

[Fujii Hironori]

Created attachment 310356 [details]
WIP patch

[Fujii Hironori]

Created attachment 310841 [details]
test case

This WIP patch doesn't solve this bug perfectly. Images which is decoding can have 0 dimension.
I'll take the (2) approach.

[Fujii Hironori]

Created attachment 310842 [details]
Patch

WIP patch

[Fujii Hironori]

Created attachment 310846 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 310846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310846&action=review

> Source/WebCore/ChangeLog:11
> +        A custom cursor image can be zero dimension if it is loading or
> +        decoding. Cursor::ensurePlatformCursor of Windows port should

I think this statement is inaccurate. An image can have zero dimension if the resource loader does not receive enough data to tell the size. In case of PNG this is only 33 bytes: 8-bytes for the signature and 25-bytes for the IHDR chunk. See https://en.wikipedia.org/wiki/Portable_Network_Graphics#File_header.

> LayoutTests/http/tests/css/cursor-image-loading.html:7
> +        cursor: url("/resources/load-and-stall.php?name=square100.png&stallAt=5000&stallFor=10"), crosshair;

stallFor=10: Why do we have to stall for 10 "seconds"?
stallAt=5000: 5000 is a large number for an image to know its size. If the test fails without your patch, I think this happens because the image has not started loading yet.

> LayoutTests/http/tests/css/cursor-image-loading.html:14
> +window.addEventListener("DOMContentLoaded", () => {

When "DOMContentLoaded" fires, the HTML document is loaded and the DOM tree is built but the external resources and stylesheets may be not yet loaded. So I think if we set the cursor css to be simply:

cursor: url("name=square100.png"), crosshair;

m_image->isNull() will be true also if eventSender.mouseMoveTo() is called form "DOMContentLoaded" event handler. Do we have this test to be a http test and have the image be loaded via l"load-and-stall.php"?

[Fujii Hironori]

Thank you so much for the review, Said.
You are right. 5000 bytes is too big. This can be tested with a non-http test.

And, I found that Qt port had same issue (Bug 68223). And, its test case can reproduce this bug with fixing CSS error in it.
I think old CSS parser accept the CSS error.

[Fujii Hironori]

Created attachment 310977 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 310977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310977&action=review

> Source/WebCore/platform/win/CursorWin.cpp:-261
> -        m_platformCursor = createSharedCursor(m_image.get(), m_hotSpot);
> -        if (!m_platformCursor)

This check was always evaluated to false. createSharedCursor() returns a Ref pointer which means m_platformCursor was never set to nullptr. So it is good to fix this code.

> Source/WebCore/platform/win/CursorWin.cpp:261
>              m_platformCursor = loadSharedCursor(0, IDC_ARROW);

Don't we have a way to load the fallback cursor if the image failed to load or was not loaded yet? For example, I would expect we load IDC_CROSS cursor not IDC_ARROW for the layout test below, if eventSender.mouseMoveTo() was executed too early.

[Fujii Hironori]

(In reply to Said Abou-Hallawa from comment #8)
> Don't we have a way to load the fallback cursor if the image failed to load
> or was not loaded yet? For example, I would expect we load IDC_CROSS cursor
> not IDC_ARROW for the layout test below, if eventSender.mouseMoveTo() was
> executed too early.

WebKit uses the subsequent cursor only if the preceding cursor has loading error or decoding error.
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/EventHandler.cpp?rev=216497#L1440

WebKit waits finishing loading and decoding of the cursor image.

On the otherhand, Firefox behaves differently. It shows following specified cursor while loading preceding cursor images.
For example,

> cursor: url(load-and-stall.php?name=x.png&stallFor=10), url(load-and-stall.php?name=y.png&stallFor=5), crosshair;

Firefox shows crosshair first, and 5 seconds later y.png, and 10 seconds later x.png.

[Build Bot]

Comment on attachment 310977 [details]
Patch

Rejecting attachment 310977 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Fujii Hironori]

Said, thank you for r+. I can't cq+. Please cq+.

[WebKit Commit Bot]

Comment on attachment 310977 [details]
Patch

Clearing flags on attachment: 310977

Committed r217406: <http://trac.webkit.org/changeset/217406>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32479837>