332713 - Crash [@ nsWindow::DataToAData] CSS custom cursors

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[Jonathan Watt [:jwatt]]

In the CSS cursor demo at http://www.gtalbot.org/DHTMLSection/Cursors.html mousing over the "<URI>" cell in the left hand column of the table causes trunk firefox to crash. The custom cursor support seems to have stopped working for me (see http://jwatt.org/svg/demos/custom-cursor.svg ) so I was looking for other examples to check it wasn't just SVG.

 	nsWindow::DataToAData(unsigned char * aImageData=0x04910fe8, unsigned int aImageBytesPerRow=128, unsigned char * aAlphaData=0x00000000, unsigned int aAlphaBytesPerRow=128, unsigned int aWidth=32, unsigned int aHeight=32) Line 2623 + 0x6	C++
 	nsWindow::SetCursor(imgIContainer * aCursor=0x03e84ea0, unsigned int aHotspotX=0, unsigned int aHotspotY=0) Line 2832 + 0x1d	C++
 	nsEventStateManager::SetCursor(int aCursor=3, imgIContainer * aContainer=0x03e84ea0, int aHaveHotspot=0, float aHotspotX=0.00000000, float aHotspotY=0.00000000, nsIWidget * aWidget=0x04894bf4, int aLockCursor=0) Line 2576 + 0x1b	C++
 	nsEventStateManager::UpdateCursor(nsPresContext * aPresContext=0x047e7068, nsEvent * aEvent=0x0012f578, nsIFrame * aTargetFrame=0x048ef7a4, nsEventStatus * aStatus=0x0012f34c) Line 2407	C++
 	nsEventStateManager::PreHandleEvent(nsPresContext * aPresContext=0x047e7068, nsEvent * aEvent=0x0012f578, nsIFrame * aTargetFrame=0x048ef7a4, nsEventStatus * aStatus=0x0012f34c, nsIView * aView=0x047f7940) Line 511	C++
 	PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f578, nsIView * aView=0x047f7940, nsEventStatus * aStatus=0x0012f34c) Line 6114 + 0x34	C++
 	PresShell::HandlePositionedEvent(nsIView * aView=0x047f7940, nsIFrame * aTargetFrame=0x048ef7a4, nsGUIEvent * aEvent=0x0012f578, nsEventStatus * aEventStatus=0x0012f34c) Line 5999 + 0x14	C++
 	PresShell::HandleEvent(nsIView * aView=0x047f7940, nsGUIEvent * aEvent=0x0012f578, nsEventStatus * aEventStatus=0x0012f34c) Line 5827 + 0x1b	C++
 	nsViewManager::HandleEvent(nsView * aView=0x047f7940, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012f578, int aCaptured=0) Line 1712	C++
 	nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f578, nsEventStatus * aStatus=0x0012f46c) Line 1665 + 0x22	C++
 	HandleEvent(nsGUIEvent * aEvent=0x0012f578) Line 174	C++
 	nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f578, nsEventStatus & aStatus=nsEventStatus_eIgnore) Line 1162 + 0xa	C++
 	nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f578) Line 1183	C++
 	nsWindow::DispatchMouseEvent(unsigned int aEventType=300, unsigned int wParam=0, long lParam=21561402) Line 6256 + 0x18	C++
 	ChildWindow::DispatchMouseEvent(unsigned int aEventType=300, unsigned int wParam=0, long lParam=21561402) Line 6439	C++
 	nsWindow::ProcessMessage(unsigned int msg=512, unsigned int wParam=0, long lParam=21561402, long * aRetValue=0x0012fac4) Line 4643 + 0x1e	C++
 	nsWindow::WindowProc(HWND__ * hWnd=0x02450810, unsigned int msg=512, unsigned int wParam=0, long lParam=21561402) Line 1351 + 0x1b	C++

Comment 1

[Jonathan Watt [:jwatt]]

You need to mouse over the cell *after* the page has completed loading to get the crash BTW.

Comment 2

[Martijn Wargers (dead)]

Did this regress between 2006-03-24 and 2006-03-25?
In that case it's likely a regression from bug 331298.

Comment 3

[Jonathan Watt [:jwatt]]

I noticed custom cursors had stopped working in my SVG demo at least a week or two ago. The first time I've ever loaded the doc that caused the crash was today (got it from google).

Comment 4

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #2)
>
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-03-24+11%3A00&maxdate=2006-03-24+21%3A00

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, bug 331298 likely broke this

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: fix crash

Fix crash only. Cursor transparency will not be handled correctly, but that is a different bug (bug 331671).

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

> Cursor transparency will not be handled correctly

what about non-ICO/CUR cursors?

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: fix non-ICO/CUR cursor crash

We can't use non-ICO/CUR cursor until nsThebesImage support the de-optimizing image. However we should not crash, of course.
nsThebesImage::LockImagePixels should fail. Otherwise, we can't determine whether image is optimized.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: get the pixels back (checked in)

this should work for getting the pixels back. I didn't test this though, only verified that it compiles.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Now works with non-ICO/CUR cursor

Removed useless pixelformat check. Thebes always assume BGRA pixelformat while returns the bogus pixelformat value.
DataToBitmap changed to use topdown DIB. Thebes also uses topdown.
Note that this does not include nsThebesImage anymore. Please use with biesi's excellent patch.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217353 [details] [diff] [review]
get the pixels back (checked in)

ok, as this seems to be working, requesting review :)

gfx/src/thebes doesn't require sr either, right?

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to comment #11)
> gfx/src/thebes doesn't require sr either, right?
Per <http://www.mozilla.org/owners.html>, gfx/src/thebes belongs to Cairo and Thebes.
See also bug 316845 comment #18. r= from pavlov or vlad means r+sr as far as cairo and thebes are concerned.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217492 [details] [diff] [review]
Now works with non-ICO/CUR cursor

+          *imageRow = aImageData + 3;

mmm. so, strictly speaking this is not correct. but windows only runs on little-endian platforms anyway, afaik, so it's ok.

can you add a comment though? (that this would need changes for big-endian platforms)


(hrm. I wonder if this would need alpha unpremultiplying...)

hm...
actually, can't a ThebesImage be in RGB32 format, i.e. without alpha data?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217353 [details] [diff] [review]
get the pixels back (checked in)

checked in this patch:

Checking in gfx/src/thebes/nsThebesImage.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v  <--  nsThebesImage.cpp
new revision: 1.12; previous revision: 1.11
done

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Added comment

> (hrm. I wonder if this would need alpha unpremultiplying...)
Windows seems to expect premultiplied alpha.

> actually, can't a ThebesImage be in RGB32 format, i.e. without alpha data?
That's a reason why I asked pavlov for review. But I believe thebes fills 0xFF in that case.

Comment 16

[Stuart Parmenter]

so i think what you really want to do is in cairo land just get the gfxASurface from the nsIImage (I recently added a method for this) then create a memory DC and select in the DIB you created here and then create a gfxWindowsSurface and just paint the nsIImage one in to the DIB-backed one and use that.  Then there would be no more shifts or loops or anything in the windows code.

24bpp surfaces just use padding for the A and should always be filled with 0xFF.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

eh, that 0xFF filling isn't guaranteed, is it?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 217606 [details] [diff] [review]
Added comment

+          //XXX this is valid only on little-endian platforms

missing space between // and XXX... also, I wouldn't put this amidst this declaration

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: Completely rewritten per pav's advice

> +          //XXX this is valid only on little-endian platforms
This comment is gone.

Comment 20

[Masatoshi Kimura [:emk]]

Comment on attachment 217637 [details] [diff] [review]
Completely rewritten per pav's advice

Sorry, this doesn't work with 16-bit color mode.

Comment 21

[Masatoshi Kimura [:emk]]

I gave up. Fell free to take it.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

*** Bug 335678 has been marked as a duplicate of this bug. ***

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

Comment 24

[Stuart Parmenter]

Attachment: fix

This may not work on win2k.. need to test there, but it won't crash.  we should get this in.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 244373 [details] [diff] [review]
fix

can you add fewer empty lines? :)

+  nsresult rv;

would be nice to move this to the line where it's used

vlad confirms that you can't make assumptions about the X byte in XRGB, which means that this patch doesn't handle bitmaps without alpha correctly

Comment 26

[Stuart Parmenter]

Attachment: fix

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 244376 [details] [diff] [review]
fix

+  HBITMAP bmp = DataToBitmap(bottomUpData, width, height, 32);

don't you need to check this for failure?

Comment 28

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061102 Minefield/3.0a1 ID:2006110214 [cairo]

verified, no crash anymore

Comment 29

[Gérard Talbot]

*** Bug 359333 has been marked as a duplicate of this bug. ***

Comment 30

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-11-17-08 of SeaMonkey trunk under Windows XP using the testcases at http://www.gtalbot.org/DHTMLSection/Cursors.html, http://jwatt.org/svg/demos/custom-cursor.svg, and https://bugzilla.mozilla.org/attachment.cgi?id=220021; none of them crash.