420786 - Setting image as desktop background causes a crash in Linux

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect, P2)

Description

[Al Billings [:abillings - ex-MoCo]]

Attachment: 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.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

Attachment: Crash .extra file

Comment 2

[Al Billings [:abillings - ex-MoCo]]

To be clear, this is Linux only.

Comment 3

[(not currently active) Ted Mielczarek]

Al: you may need to follow the instructions in bug 407748 to get the right CA certs for Ubuntu to upload crash reports.

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Thanks, Ted. I didn't realize that was what was blocking things. I'll do that today, given time.

Comment 5

[Sylvain Pasche]

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.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: 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.

Comment 7

[Sylvain Pasche]

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)"

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Well the thing is, Firefox 2 only supports 3 or 6 but works somehow.  I'm not sure why...

Comment 9

[Sylvain Pasche]

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

[Michael Ventnor]

Hmm, the stack suggests that this a Cairo crash...

Comment 11

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

Comment on attachment 308420 [details] [diff] [review]
Patch to support GConf's actual format

Need more info as to why this is needed.

Comment 12

[Ian Spence]

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

Comment 13

[Sylvain Pasche]

I found some unbalanced gfxIImageFrame::LockImageData/UnLockImageData in nsGNOMEShellServer.cpp. patch coming.

Comment 14

[Sylvain Pasche]

Attachment: 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.

Comment 15

[Sylvain Pasche]

I opened bug 421977 for the GConf change.

Comment 16

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

Shouldn't you be adding the LockSurface() rather than removing the UnlockSurface?

Comment 17

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

Er, LockImageData(), I mean.

Comment 18

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

Comment on attachment 308498 [details] [diff] [review]
patch

Someone who understands the GFX APIs should review this - vlad or stuart?

Comment 19

[Sylvain Pasche]

(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.

Comment 20

[Sylvain Pasche]

Attachment: 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

Comment 21

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

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.?

Comment 22

[Sylvain Pasche]

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.

Comment 23

[Sylvain Pasche]

Attachment: 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?

Comment 24

[Sylvain Pasche]

Attachment: real one

Comment 25

[Reed Loden [:reed]]

(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.

Comment 26

[Sylvain Pasche]

Attachment: 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)

Comment 27

[Sylvain Pasche]

Attachment: now with a browser test

Comment 28

[Sylvain Pasche]

Attachment: 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.

Comment 29

[Sylvain Pasche]

Attachment: version with setTimeout updated

I forgot to close the tab and removed an unused variable.

Comment 30

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

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.

Comment 31

[Reed Loden [:reed]]

Comment on attachment 309564 [details] [diff] [review]
using onload listener instead of a timeout

Crash fix on Linux for a blocker. Includes test!

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 309564 [details] [diff] [review]
using onload listener instead of a timeout

a=beltzner

Comment 33

[Reed Loden [:reed]]

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

Comment 34

[Al Billings [:abillings - ex-MoCo]]

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.