Bug 78834

Summary: Chromium Mac: mouse cursors set in CSS often don't display
Product: WebKit Reporter: Avi Drissman <avi>
Component: PlatformAssignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Major CC: dan, eric, jamesr, japhet, rniwa, senorblanco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.6   
Attachments:
Description Flags
patch abarth: review+

Description Avi Drissman 2012-02-16 13:16:33 PST
This is http://code.google.com/p/chromium/issues/detail?id=111027 and possibly http://code.google.com/p/chromium/issues/detail?id=114517. This continues the discussion from '027.

To repro:
- Load http://maps.google.com/ but do not mouse over the page.
- When the page is done loading, set a breakpoint on the last line of CachedImage::imageForRenderer, where Image::nullImage() is returned.
- Mouse over the page, continue over the breakpoint until it is called from EventHandler::selectCursor

Inside of EventHandler::selectCursor, it looks to see if the CSS style has a cursor set:

    if (style && style->cursors()) {
        const CursorList* cursors = style->cursors();
        for (unsigned i = 0; i < cursors->size(); ++i) {
            CachedImage* cimage = 0;
            StyleImage* image = (*cursors)[i].image();
            if (image && image->isCachedImage())
                cimage = static_cast<StyleCachedImage*>(image)->cachedImage();
            if (!cimage)
                continue;
            IntPoint hotSpot = (*cursors)[i].hotSpot();
            // Limit the size of cursors so that they cannot be used to cover UI elements in chrome.
            IntSize size = cimage->imageForRenderer(renderer)->size(); // ***1***
            if (size.width() > 128 || size.height() > 128)
                continue;
            if (!cimage->errorOccurred()) // ***2***
                return Cursor(cimage->imageForRenderer(renderer), hotSpot); // ***3***
        }
    }

In my repro case, there is one item in the |cursors| list. Indeed, the type is a CachedImage. Inside of CachedImage, errorOccurred() returns false, but there is no m_image. So Image::nullImage() is returned from CachedImage::imageForRenderer (line 1)

Its size is 0x0, so it passes the tests, and since no error occurred (line 2), a bogus cursor is created (line 3).

What is going on here? Why are the cached images not loading?
Comment 1 Avi Drissman 2012-02-16 13:39:34 PST
BTW, along the lines of blaming caching, another repro is:

http://www.w3schools.com/cssref/playit.asp?filename=playcss_cursor&preval=url(smiley.gif),url(myBall.cur),auto

Watch it work. Let it sit for twenty minutes. The cursor no longer works.
Comment 2 James Robinson 2012-02-16 13:40:15 PST
Do any of you know what set of states that corresponds to in CachedImage?
Comment 3 Nate Chapin 2012-02-16 13:51:51 PST
(In reply to comment #2)
> Do any of you know what set of states that corresponds to in CachedImage?

I think this is the same root cause as https://bugs.webkit.org/show_bug.cgi?id=73760
Comment 4 Avi Drissman 2012-02-16 16:33:05 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Do any of you know what set of states that corresponds to in CachedImage?
> 
> I think this is the same root cause as https://bugs.webkit.org/show_bug.cgi?id=73760

It's possibly related, but the patch you posted didn't change this.
Comment 5 Nate Chapin 2012-02-17 11:18:45 PST
Created attachment 127615 [details]
patch
Comment 6 Adam Barth 2012-02-17 11:36:29 PST
Comment on attachment 127615 [details]
patch

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

This change looks very reasonable.  I wish he had a test for the change.  I talked with Nate, and he's been working on trying to reduce the bug into a test for a while without success.  If anyone thoughts on how to write a test, please let us know.

> Source/WebCore/rendering/style/StyleCachedImage.h:59
> +    StyleCachedImage(CachedImage*);

explicit
Comment 7 Avi Drissman 2012-02-17 11:40:22 PST
The problem with getting a reduced testcase is that the bug is that images get evicted under cache pressure. If there's a way to force the cache to clean itself out that might help.
Comment 8 Nate Chapin 2012-02-17 11:46:12 PST
http://trac.webkit.org/changeset/108100
Comment 9 Eric Seidel (no email) 2012-02-17 11:59:17 PST
Maybe layoutTestController.setCacheModel would help?
Comment 10 Nate Chapin 2012-02-17 14:09:03 PST
*** Bug 73760 has been marked as a duplicate of this bug. ***