88.21 KB, text/plain
393 bytes, application/octet-stream
7.46 KB, patch
|Details | Diff | Splinter Review|
7.35 KB, patch
|Details | Diff | Splinter Review|
Created attachment 307129 [details] dmp file of crash When a user attempts to set an image as a desktop background, Firefox will crash. This was found on Ubuntu 7.10. Steps to Reproduce 1. Navigate to an image file (such as http://l.yimg.com/www.flickr.com/images/home_photo_nuomi.jpg) 2. Right click on the image and choose "Set as Desktop Background" in context menu. 3. In the "Set Desktop Background" dialog, select "Set Desktop Background". Result: Crash As an aside, the crash reporter never uploads the crash (it just goes into the pending directory) so I've attached it here. This was found in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008030304 Minefield/3.0b4pre.
To be clear, this is Linux only.
Al: you may need to follow the instructions in bug 407748 to get the right CA certs for Ubuntu to upload crash reports.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Thanks, Ted. I didn't realize that was what was blocking things. I'll do that today, given time.
I couldn't reproduce the crash but I see another strangeness (also on Ubuntu 7.10): in my gconf database, colors are encoded as #rrrrggggbbbb while nsGNOMEShellService::GetDesktopBackgroundColor apparently expects them in #rgb or #rrggbb (so I'm getting at [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIShellService.desktopBackgroundColor]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://browser/content/setDesktopBackground.js :: anonymous :: line 57" data: no]). I'll try to investigate a bit more and file another bug.
Created attachment 308420 [details] [diff] [review] Patch to support GConf's actual format GConf had this format for a while though. Using Firefox 2 here works, but I can't see the Firefox 3 crash without this patch.
Component: General → File Handling
QA Contact: general → file.handling
Component: File Handling → OS Integration
QA Contact: file.handling → os.integration
should the 9-character format be supported too? The background color is parsed using gdk_color_parse (): "The text string can be in any of the forms accepted by XParseColor" In XParseColor: " The syntax is an initial sharp sign character followed by a numeric specification, in one of the following formats: #RGB (4 bits each) #RRGGBB (8 bits each) #RRRGGGBBB (12 bits each) #RRRRGGGGBBBB (16 bits each)"
Well the thing is, Firefox 2 only supports 3 or 6 but works somehow. I'm not sure why...
I can now reproduce the crash with another profile (not sure what I did wrong when I tried previously). It seems that the GetDesktopBackgroundColor failure has nothing to do with this crash. I'm getting the same failure in Error Console with branch. #0 0x00007f34fd2e0ac0 in gfxASurface::CairoSurface (this=0x0) at ../../../dist/include/thebes/gfxASurface.h:99 #1 0x00007f34fd2f15e2 in gfxPattern (this=0x1f02080, surface=0x0) at /home/sypasche/moz/trunk/mozilla/gfx/thebes/src/gfxPattern.cpp:59 #2 0x00007f34fa8bc2b8 in nsThebesImage::Draw (this=0x1939c10, aContext=@0x19f7520, aSourceRect=@0x7fff14fc10f0, aDestRect=@0x7fff14fc10b0) at /home/sypasche/moz/trunk/mozilla/gfx/src/thebes/nsThebesImage.cpp:527 #3 0x00007f34fb29cb14 in nsLayoutUtils::DrawImage (aRenderingContext=0x19f7520, aImage=0x1d9df60, aDestRect=@0x7fff14fc1280, aDirtyRect=@0x7fff14fc1270, aSourceRect=0x0) at /home/sypasche/moz/trunk/mozilla/layout/base/nsLayoutUtils.cpp:2419 #4 0x00007f34fb4a6e2c in nsImageBoxFrame::PaintImage (this=0x18ac118, aRenderingContext=@0x19f7520, aDirtyRect=@0x7fff14fc13a0, aPt=@0x7fff14fc12f0) at /home/sypasche/moz/trunk/mozilla/layout/xul/base/src/nsImageBoxFrame.cpp:382 #5 0x00007f34fb4a6eab in nsDisplayXULImage::Paint (this=0x285b6e0, aBuilder=0x7fff14fc14c0, aCtx=0x19f7520, aDirtyRect=@0x7fff14fc13a0) at /home/sypasche/moz/trunk/mozilla/layout/xul/base/src/nsImageBoxFrame.cpp:335 #6 0x00007f34fb277693 in nsDisplayList::Paint (this=0x2adb478, aBuilder=0x7fff14fc14c0, aCtx=0x19f7520, aDirtyRect=@0x7fff14fc13a0) at /home/sypasche/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:294 #7 0x00007f34fb2776df in nsDisplayWrapList::Paint (this=0x2adb460, aBuilder=0x7fff14fc14c0, aCtx=0x19f7520, aDirtyRect=@0x7fff14fc13a0) at /home/sypasche/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:691 #8 0x00007f34fb27775e in nsDisplayClip::Paint (this=0x2adb460, aBuilder=0x7fff14fc14c0, aCtx=0x19f7520, aDirtyRect=@0x7fff14fc1a10) at /home/sypasche/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:883 #9 0x00007f34fb277693 in nsDisplayList::Paint (this=0x7fff14fc1490, aBuilder=0x7fff14fc14c0, aCtx=0x19f7520, aDirtyRect=@0x7fff14fc1a10) at /home/sypasche/moz/trunk/mozilla/layout/base/nsDisplayList.cpp:294 #10 0x00007f34fb29fcc5 in nsLayoutUtils::PaintFrame (aRenderingContext=0x19f7520, aFrame=0x12e7fc0, aDirtyRegion=@0x7fff14fc19e0, aBackground=4293651693) at /home/sypasche/moz/trunk/mozilla/layout/base/nsLayoutUtils.cpp:875 #11 0x00007f34fb2b1d32 in PresShell::Paint (this=0x12e6000, aView=0x12da6b0, aRenderingContext=0x19f7520, aDirtyRegion=@0x7fff14fc19e0) at /home/sypasche/moz/trunk/mozilla/layout/base/nsPresShell.cpp:5392 #12 0x00007f34fb771439 in nsViewManager::RenderViews (this=0x12da610, aView=0x12da6b0, aRC=@0x19f7520, aRegion=@0x7fff14fc1ac0) at /home/sypasche/moz/trunk/mozilla/view/src/nsViewManager.cpp:602 #13 0x00007f34fb7720ab in nsViewManager::Refresh (this=0x12da610, aView=0x12da6b0, aContext=0x19f7520, aRegion=0x176e990, aUpdateFlags=1) at /home/sypasche/moz/trunk/mozilla/view/src/nsViewManager.cpp:491 #14 0x00007f34fb7726f9 in nsViewManager::DispatchEvent (this=0x12da610, aEvent=0x7fff14fc1d90, aStatus=0x7fff14fc1d2c) at /home/sypasche/moz/trunk/mozilla/view/src/nsViewManager.cpp:1132 #15 0x00007f34fb768ab3 in HandleEvent (aEvent=0x7fff14fc1d90) at /home/sypasche/moz/trunk/mozilla/view/src/nsView.cpp:168 #16 0x00007f34fda29ab1 in nsCommonWidget::DispatchEvent (this=0x12e5040, aEvent=0x7fff14fc1d90, aStatus=@0x7fff14fc1f9c) at /home/sypasche/moz/trunk/mozilla/widget/src/gtk2/nsCommonWidget.cpp:153 #17 0x00007f34fda1d6fe in nsWindow::OnExposeEvent (this=0x12e5040, aWidget=0x11e6f00, aEvent=0x7fff14fc26d0) at /home/sypasche/moz/trunk/mozilla/widget/src/gtk2/nsWindow.cpp:1763
Hmm, the stack suggests that this a Cairo crash...
Comment on attachment 308420 [details] [diff] [review] Patch to support GConf's actual format Need more info as to why this is needed.
so I don't know enough to say anything, but backing up through those calls got me to mozilla/gfx/thebes/src/gfxImageSurface.cpp I believe that for some reason mImageSurface is null at the time we try to use it So throwing that out there as a possible place to look
I found some unbalanced gfxIImageFrame::LockImageData/UnLockImageData in nsGNOMEShellServer.cpp. patch coming.
Created attachment 308498 [details] [diff] [review] patch so nsGNOMEShellService is calling gfxIImageFrame::UnLockImageData but never calls gfxIImageFrame::LockImageData before. gfxIImageFrame::UnLockImageData is resetting mOptSurface. Both mOptSurface and mImageSurface are null when gfxASurface::ThebesSurface is called later during painting, so it is returning a null surface which leads to the crash. Maybe it crashes since bug 415854 which changed things related to surfaces.
Assignee: nobody → sylvain.pasche
Status: NEW → ASSIGNED
I opened bug 421977 for the GConf change.
Shouldn't you be adding the LockSurface() rather than removing the UnlockSurface?
Er, LockImageData(), I mean.
Comment on attachment 308498 [details] [diff] [review] patch Someone who understands the GFX APIs should review this - vlad or stuart?
Whiteboard: [has patch][needs review gavin] → [has patch]
(In reply to comment #16) > Shouldn't you be adding the LockImageData() rather than removing the > UnlockSurface? Apparently locking is not required for Thebes Images (for instance this caller does not call Lock/Unlock http://mxr.mozilla.org/mozilla/source/widget/src/gtk2/nsWindow.cpp#1038). But yes, it might be better not to make this assumption and use locking there.
Created attachment 308994 [details] [diff] [review] use LockImageData/UnlockImageData I've added a test. This patch requires an image in browser/components/shell/test/mlogosm.gif which I copied from toolkit/components/passwordmgr/test/mlogosm.gif
Attachment #308498 - Attachment is obsolete: true
vlad/joe/pav say the locking isn't needed, so let's just go with the first patch. Could you just test SetDesktopBackground directly rather than having the test launch the dialog, etc.?
ok. I tried using SetDesktopBackground directly before, but I couldn't reproduce the crash. I'll check again if I can simulate what is done in the dialog.
Created attachment 309197 [details] [diff] [review] no locking, with test I could get the crash without using the dialog. I attached the patch in git format to get the binary included. What's the recommended way to deal with binary files in patches?
Created attachment 309198 [details] [diff] [review] real one
(In reply to comment #23) > I could get the crash without using the dialog. I attached the patch in git > format to get the binary included. What's the recommended way to deal with > binary files in patches? Attach the binary files as separate attachments.
Created attachment 309205 [details] image used by test Thanks reed. This should go in ./browser/components/shell/test/mlogosm.gif (it can also be copied from ./toolkit/components/passwordmgr/test/mlogosm.gif)
Created attachment 309538 [details] [diff] [review] now with a browser test
Created attachment 309564 [details] [diff] [review] using onload listener instead of a timeout I came across a strange behavior. If you load this test multiple times, it will only finish on the first run. Apparently image document don't fire load events if they are loaded more than once (if you close the tab and open it again with the same image). Not sure if this is considered a bug, but a consequence is that using events may not be the safest way here.
Created attachment 309567 [details] [diff] [review] version with setTimeout updated I forgot to close the tab and removed an unused variable.
Comment on attachment 309564 [details] [diff] [review] using onload listener instead of a timeout I think we should go with this approach. We can revisit if it becomes an issue.
Attachment #309564 - Flags: review+
Comment on attachment 309564 [details] [diff] [review] using onload listener instead of a timeout Crash fix on Linux for a blocker. Includes test!
Attachment #309564 - Flags: approval1.9b5?
Comment on attachment 309564 [details] [diff] [review] using onload listener instead of a timeout a=beltzner
Attachment #309564 - Flags: approval1.9b5? → approval1.9b5+
Whiteboard: [has patch] → [has patch][has review][has test][has approval]
Target Milestone: --- → Firefox 3 beta5
Checking in browser/components/shell/Makefile.in; /cvsroot/mozilla/browser/components/shell/Makefile.in,v <-- Makefile.in new revision: 1.3; previous revision: 1.2 done Checking in browser/components/shell/src/nsGNOMEShellService.cpp; /cvsroot/mozilla/browser/components/shell/src/nsGNOMEShellService.cpp,v <-- nsGNOMEShellService.cpp new revision: 1.22; previous revision: 1.21 done RCS file: /cvsroot/mozilla/browser/components/shell/test/Makefile.in,v done Checking in browser/components/shell/test/Makefile.in; /cvsroot/mozilla/browser/components/shell/test/Makefile.in,v <-- Makefile.in initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/components/shell/test/browser_420786.js,v done Checking in browser/components/shell/test/browser_420786.js; /cvsroot/mozilla/browser/components/shell/test/browser_420786.js,v <-- browser_420786.js initial revision: 1.1 done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has test][has approval]
Bug repros in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031904 Minefield/3.0b5pre and does not repro in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008032104 Minefield/3.0b5pre with same profile. Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.