Closed Bug 1315443 Opened 3 years ago Closed 3 years ago

Some customized cursors could not work on Windows Firefox

Categories

(Core :: ImageLib, defect)

49 Branch
All
Windows 10
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ttt43ttt, Assigned: aosmond)

Details

(Keywords: testcase)

Attachments

(3 files, 3 obsolete files)

Attached image test.cur
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36

Steps to reproduce:

I set the customized cursors on a div using CSS "cursor:url(xxx.cur), default".


Actual results:

Some cursors work well, but some of them don't work, and they fallback to the default.
All of these cursors can work on Chrome, IE, Safari, and Mac Firefox.
I find that if I change the hotspot of the broken *.cur file, they can work under some values. So this issue may relate to the hotspot of the cursor file.


Expected results:

The cursors should work.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Keywords: testcase
OS: Unspecified → Windows 10
Hardware: Unspecified → All
Attached file testcase (obsolete) —
Component: Untriaged → DOM
Product: Firefox → Core
Component: DOM → CSS Parsing and Computation
Attached file probably better testcase (obsolete) —
The testcase in comment 1 doesn't really show the issue because <body> doesn't have height by default in Firefox (unlike in Chrome and Safari).

This testcase sets the height of <body> to be viewport, and also inlines the cur file and uses pointer as fallback so that it is clearer that fallback happens (rather than falling to the default value).
The issue is that, in nsFrame::FillCursorInformationFromStyle, image request has STATUS_ERROR bit set, which indicates it is an issue in the image decoder which fails to decode this cur file.
Component: CSS Parsing and Computation → ImageLib
This is a testcase which simply uses <img> tag to display the cur file. It shows the image correctly on Chrome and Safari, but shows a broken mark on Firefox.
Attachment #8807848 - Attachment is obsolete: true
Attachment #8810164 - Attachment is obsolete: true
Assignee: nobody → aosmond
Looks like we are tripping up on the image data offset in the BMP header being the same as the start of the color table: http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/image/decoders/nsBMPDecoder.cpp#724
Attached patch Ignore offset, v1 (obsolete) — Splinter Review
Further analysis required before review/landing (ideally dig up some documentation somewhere saying this is the right thing to do...). It appears that if we ignore the data offset in the BMP header, and assume the data follows the color table directly if inside an ICO, we can decode the image just fine. This is what Chrome does.
Consistently use the BMP's BPP value instead of the ICO's when it becomes available, as we already acknowledge it can be wrong. In this case it affected our offset calculation.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f0212241a2e241f11e8de90fb28aaaf82f9364d
Attachment #8810537 - Attachment is obsolete: true
Attachment #8811255 - Flags: review?(tnikkel)
Comment on attachment 8811255 [details] [diff] [review]
Prefer BMP BPP over ICO BPP, v1

I had this exact patch sitting in my patch queue forgotten!

I'm confident it's correct because this is the comment I had on the patch:

"Use the bits per pixel value we read from the bmp header inside icos, not the value in the ico directory entry.

If they disagree we want to use the bmp header value. Further mBitCount is a union with the cursor hot spot is this is a CUR file, so it doesn't even represent BPP in CUR files."

On cursors mBitCount isn't valid because it represents the hotspot of the cursor.

There might be more bugs where we use fields of this union incorrectly, we should have a followup for auditing that.
Attachment #8811255 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9876606891a7
Use the embedded BMPs BPP over the ICOs BPP when available. r=tnikkel
(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> Comment on attachment 8811255 [details] [diff] [review]
> Prefer BMP BPP over ICO BPP, v1
> 
> I had this exact patch sitting in my patch queue forgotten!
> 
> I'm confident it's correct because this is the comment I had on the patch:
> 
> "Use the bits per pixel value we read from the bmp header inside icos, not
> the value in the ico directory entry.
> 
> If they disagree we want to use the bmp header value. Further mBitCount is a
> union with the cursor hot spot is this is a CUR file, so it doesn't even
> represent BPP in CUR files."
> 
> On cursors mBitCount isn't valid because it represents the hotspot of the
> cursor.
> 
> There might be more bugs where we use fields of this union incorrectly, we
> should have a followup for auditing that.

The only questionable use case of mBitCount/mPlanes/mXHotspot/mYhotspot (which isn't protected by checking if it is a cursor) is here:

http://searchfox.org/mozilla-central/rev/efcb1644e49c36445e75d89b434fdf4c84832c84/image/decoders/nsICODecoder.cpp#230

If there are multiple cursors in the ICO, we won't necessarily select the biggest one because we are checking the "BPP." We shouldn't check it in that case.
https://hg.mozilla.org/mozilla-central/rev/9876606891a7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.