430506 - Firefox crash on Exit when the Adblock Plus Pref Window is open

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: crash report

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042320 Firefox/3.0pre ID:2008042320

Steps to reproduce: (thanks to bret for helping me finding this :)
-> Install Adblock Plus
-> Have some tabs open
-> Open the Adblock Plus Prefs via the Extension Manager
-> Close Firefox with the Adblock Plus Prefs open
--> Crash on Exit 

Before Firefox crashes, i get:

 Assertion failure: 0 == rv, at /debug/mozilla/nsprpub/pr/src/pthreads/ptsynch.c:207
./run-mozilla.sh: line 131: 24669 Abort trap              "$prog" ${1+"$@"} 

The Assertion and Stack looks similar to bug 429215, but this bug here has completely different steps to reproduce and bug 429215 is even fixed

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I've confirmed this.

I also confirmed that bug 429215 hasn't recurred:  I tried quitting
with only the Amazing Media Browser's settings window open, and with
that window open on top of the Extension Manager (which I hadn't
tested before) -- neither caused a crash.

I'll be working on this (probably tomorrow).

Adblock Plus is a popular extension.  I agree with Tomcat that this
should probably block 1.9.

Comment 2

[Damon Sicore (:damons)]

Does this happen only in a debug build?  If so, not a blocker.  If not, please renom.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Oops.  Should have mentioned that I tested with an opt build (today's nightly).

Comment 4

[Wladimir Palant]

I cannot reproduce it on Windows, guess this is Mac only. In general, there should be nothing special about this window. Only two things I can think about: it has a menu, and it keeps a reference to the content window that was active when you opened it. If it is the former, exiting Firefox with DOM Inspector window open should also cause a crash.

Comment 5

[Carsten Book [:Tomcat]]

changing summary per comment #3 (thanks steven btw)

Comment 6

[Damon Sicore (:damons)]

While it's popular, I'm still not convinced we should block if this was the last bug on the list.  

Any other characteristics that would indicate we should hold the release for this?  Is it exploitable?  Jessie, what do you think?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

My guess is this isn't limited to the particular case that was reported.  I'll know more once I've looked into it further (which I'm doing right now).

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I'm now pretty sure this bug has an easy fix.  I should have a patch up in an hour or so.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

> I'm now pretty sure this bug has an easy fix.

Yup, it does.

Turns out that (under some circumstances) the value of
sMenuBarX::sLastGeckoMenuBarPainted can change during
[NativeMenuItemTarget menuItemHit:].

My current patch bails when this happens.  This seems the safest thing
to do ... though in principal we could just reset the menuBar local
variable and continue.

A tryserver build will follow shortly.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with this patch:

https://build.mozilla.org/tryserver-builds/2008-04-25_16:[e-mail]-bugzilla430506/[e-mail]

Comment 11

[Carsten Book [:Tomcat]]

(In reply to comment #10)
> Here's a tryserver build made with this patch:
> 
> https://build.mozilla.org/tryserver-builds/2008-04-25_16:[e-mail]-bugzilla430506/[e-mail]
> 

This tryserver build fix the crash for me (thanks steven!). 

Another testcase where you crash with a unpatched build is with the "mid" dictionary extension (https://addons.mozilla.org/en-US/firefox/addon/524). When you close Firefox with the mid "toolbar" open, you crash also with a unpatched build, like a latest debug Build.

The tryserver build also fix this crash with the mid extension, so i think this bug is really a 1.9 blocker.

Comment 12

[Damon Sicore (:damons)]

OK, so, let's take the patch, but I still think if this were the last bug in 1.9, I wouldn't hold the release a day.  -'ing.  Again, we'll take the patch.  Request approval once reviewed.

Comment 13

[Josh Aas]

Comment on attachment 317792 [details] [diff] [review]
Fix

Yeah, this is the right thing to do.

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 317792 [details] [diff] [review]
Fix

a1.9+=damons

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk:

Checking in widget/src/cocoa/nsMenuBarX.mm;
/cvsroot/mozilla/widget/src/cocoa/nsMenuBarX.mm,v  <--  nsMenuBarX.mm
new revision: 1.69; previous revision: 1.68
done

Comment 16

[Carsten Book [:Tomcat]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008043013 Firefox/3.0pre - no crash on my steps to reproduce

--> Verified fixed