508057 - crash [@ imgRequestProxy::OnStopRequest(nsIRequest*, nsISupports*, unsigned int, int) ] Classic Compact theme

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Kevin Brosnan]

The extension developer reported that his theme is causing crashes in Firefox. BZ asked me to file a bug so this would not get lost. http://forums.mozillazine.org/viewtopic.php?f=18&t=1395115

Install the classic compact theme and Classic Compact Options

https://addons.mozilla.org/en-US/firefox/addons/versions/6969
https://addons.mozilla.org/en-US/firefox/addons/versions/3699
Use the Classic Compact Options to set the "background style" to "flat"
bookmarks > organize bookmarks will often crash Firefox

Crash stats link in the url ~75 crashes a week.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Just to be clear, this is much like bug 493240 in terms of branch/security stuff.

Comment 3

[KLB]

Most crashes in past week caused by me trying to figure out what is going on. I am getting scattered reports from other users of the crash issue.  Only crashes when Classic Compact Options "background style" option is set to either "flat" or "flat except tabs". I have not been able to cause crashes with other option combinations or with default Classic Compact theme.

I have gone through all related CSS and can not find obvious CSS errors.

Comment 4

[Samuel Sidler (old account; do not CC)]

(In reply to comment #2)
> Just to be clear, this is much like bug 493240 in terms of branch/security
> stuff.

Security-wise? Or interface-using-wise?

Comment 5

[Boris Zbarsky [:bzbarsky]]

The former; I have no idea what you mean by the latter.... ;)

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 392273 [details] [diff] [review]
Proposed patch

I still don't know of any way to ask DXR for things that call Cancel on an imgIRequest :(

Comment 7

[KLB]

If it is any help, I think I got my theme/extension combo to stop crashing Firefox 3.5.1.  What I did was better synchronize my editBookmarkOverlay.css with the default Firefox editBookmarkOverlay.css by adding the following CSS to my version:

===begin added CSS====
/* Hide the value column of the tag autocomplete popup
* leaving only the comment column visible. This is
* so that only the tag being edited is shown in the
* popup.
*/
#editBMPanel_tagsField #treecolAutoCompleteValue {
  visibility: collapse;
}


/* ::::: bookmark panel dropdown icons ::::: */

#editBMPanel_folderMenuList[selectedIndex="0"],
#editBMPanel_toolbarFolderItem {
  list-style-image: url("chrome://browser/skin/places/bookmarksToolbar.png") !important;
  -moz-image-region: auto !important;
}

#editBMPanel_folderMenuList[selectedIndex="1"],
#editBMPanel_bmRootItem {
  list-style-image: url("chrome://browser/skin/places/bookmarksMenu.png") !important;
  -moz-image-region: auto !important;
}

#editBMPanel_folderMenuList[selectedIndex="2"],
#editBMPanel_unfiledRootItem {
  list-style-image: url("chrome://browser/skin/places/unsortedBookmarks.png") !important;
  -moz-image-region: auto !important;
}
===End Added CSS===

I have no idea why this mattered and I don't notice visual changes to the bookmarks library. What I do know is that since I added this code I have been unable to get Firefox to crash regardless of how the "background style" option is set in Classic Compact Options.  A test build of Classic Compact with the above CSS added can be downloaded and tried from http://environmentalchemistry.com/classiccompact.jar.  Once I have confirmation from others that this indeed stops the crashes I will push this update to AMO.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/a7e5345ecbea

Comment 9

[Samuel Sidler (old account; do not CC)]

Boris: Is this patch ready for 1.9.1? Can you request approval if so?

Comment 10

[Samuel Sidler (old account; do not CC)]

Comment on attachment 392273 [details] [diff] [review]
Proposed patch

Approved for 1.9.1.3. a=ss

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/604b99ffe2c9

Comment 12

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

This does not appear to be fixed in 1.9.1. Using the extension and theme linked above (before they were fixed on August 4) with both 1.9.1.2 and a post-fix 1.9.1.3 build, I easily get the crash while doing organize bookmarks.

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090817 Shiretoko/3.5.3pre (.NET CLR 3.5.30729).

Comment 13

[Joe Drew (not getting mail)]

So this is now crashing because of an nsImageBoxFrame that calls Cancel() in UpdateImage(), but then apparently goes away shortly thereafter. Calling CancelAndForgetObserver() instead should work, but I'm not sure if it's 100% safe (see: bug 393936).

Comment 14

[Samuel Sidler (old account; do not CC)]

Given the comments above, this isn't actually fixed for 1.9.1.3. We'll push it to 1.9.1.4.

Comment 15

[Joe Drew (not getting mail)]

Attachment: Cancel -> CancelAndForgetObserver

Sigh. At some point we'll have solved this ownership model, honest!

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 397387 [details] [diff] [review]
Cancel -> CancelAndForgetObserver

r=bzbarsky

Comment 17

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/08c42708aad8

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 397387 [details] [diff] [review]
Cancel -> CancelAndForgetObserver

Applies cleanly to 1.9.1. Requesting approval.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 397387 [details] [diff] [review]
Cancel -> CancelAndForgetObserver

Approved for 1.9.1.4, a=dveditz

Comment 20

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0e2cdc91a769

Comment 21

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/41b236f244fd

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk, 1.9.2, and 1.9.1 with the builds below and Classic Compact theme 3.2.0 installed.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091006 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091006044117

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b1pre) Gecko/20091007 Namoroka/3.6b1pre (.NET CLR 3.5.30729) ID:20091007045631

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4) Gecko/20091007 Firefox/3.5.4 (.NET CLR 3.5.30729) ID:20091007001339

Comment 23

[Daniel Veditz [:dveditz]]

nominating for 1.9.0 consideration:
  This is a regression from bug 393936. That patch didn't land in the main Mozilla repo for 1.9.0, but that bug has a 1.9.0 back-port by asac which presumably landed in Ubuntu and maybe other linux distros. If so they need this fix (and for bug 493240) on that branch also.

Comment 24

[Daniel Veditz [:dveditz]]

Ubuntu didn't pick up the older bug, we can unhide this one.