Setting image as desktop background causes a crash in Linux

VERIFIED FIXED in Firefox 3 beta5

Status

()

P2
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: abillings, Assigned: sylvain.pasche)

Tracking

({crash})

Trunk
Firefox 3 beta5
x86
Linux
crash
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 307130 [details]
Crash .extra file
(Reporter)

Comment 2

11 years ago
To be clear, this is Linux only.
(Reporter)

Updated

11 years ago
Flags: blocking-firefox3?
Keywords: crash
Severity: normal → critical
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
(Reporter)

Comment 4

11 years ago
Thanks, Ted. I didn't realize that was what was blocking things. I'll do that today, given time.
(Assignee)

Comment 5

11 years ago
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
Attachment #308420 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

11 years ago
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...
(Assignee)

Comment 9

11 years ago
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

Comment 10

11 years ago
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.
Attachment #308420 - Flags: review?(gavin.sharp)

Comment 12

11 years ago
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
(Assignee)

Comment 13

11 years ago
I found some unbalanced gfxIImageFrame::LockImageData/UnLockImageData in nsGNOMEShellServer.cpp. patch coming.
(Assignee)

Comment 14

11 years ago
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
Attachment #308498 - Flags: review?(gavin.sharp)
(Assignee)

Comment 15

11 years ago
I opened bug 421977 for the GConf change.
Whiteboard: [has patch][needs review gavin]
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?
Attachment #308498 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][needs review gavin] → [has patch]
(Assignee)

Comment 19

11 years ago
(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.
(Assignee)

Comment 20

11 years ago
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.?
(Assignee)

Comment 22

11 years ago
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.
(Assignee)

Comment 23

11 years ago
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?
Attachment #308420 - Attachment is obsolete: true
Attachment #308994 - Attachment is obsolete: true
Attachment #309197 - Flags: review?(gavin.sharp)
(Assignee)

Comment 24

11 years ago
Created attachment 309198 [details] [diff] [review]
real one
Attachment #309197 - Attachment is obsolete: true
Attachment #309198 - Flags: review?(gavin.sharp)
Attachment #309197 - Flags: review?(gavin.sharp)
(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.
(Assignee)

Comment 26

11 years ago
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)
(Assignee)

Comment 27

11 years ago
Created attachment 309538 [details] [diff] [review]
now with a browser test
Attachment #309198 - Attachment is obsolete: true
Attachment #309205 - Attachment is obsolete: true
Attachment #309538 - Flags: review?(gavin.sharp)
Attachment #309198 - Flags: review?(gavin.sharp)
(Assignee)

Comment 28

11 years ago
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.
(Assignee)

Comment 29

11 years ago
Created attachment 309567 [details] [diff] [review]
version with setTimeout updated

I forgot to close the tab and removed an unused variable.
Attachment #309538 - Attachment is obsolete: true
Attachment #309567 - Flags: review?(gavin.sharp)
Attachment #309538 - Flags: review?(gavin.sharp)
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?
Attachment #309567 - Flags: review?(gavin.sharp)
Comment on attachment 309564 [details] [diff] [review]
using onload listener instead of a timeout

a=beltzner
Attachment #309564 - Flags: approval1.9b5? → approval1.9b5+
Keywords: checkin-needed
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
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has test][has approval]
(Reporter)

Comment 34

11 years ago
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
Depends on: 499435
No longer blocks: 483382
Depends on: 483382
You need to log in before you can comment on or make changes to this bug.