325279 - Firefox 1.6a1 crash [@ ClassifyWrapper] on exit (extension related)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Stellan Klebom]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060130 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060130 Firefox/1.6a1

I have 51 Firefox extensions in use.

I've noted crashes on exit for 1.6a1 nightlies for weeks now, and decided to find out if an extension was to blame...

After a lot enabling/disabling extensions I found out having all four of

 IE View 1.2.7
 Launchy 4.2.0
 Greasmonkey 0.6.4
 SessionSaver .2 d1 * nightly 30

enabled caused Firefox to crash on exit. Disabling anyone of them makes the problem go away. This with all other 47 either enabled or disabled.

Reproducible: Always

Steps to Reproduce:
1. Just install and enable all for extensions in details
2. quit firefox application
Actual Results:  
crash

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Any chance you could start pulling out pieces of those extensions to narrow down what exactly they're doing?

Comment 2

[Adam Guthrie]

Could you install with talkback and get a talkback ID for the crash? http://kb.mozillazine.org/Talkback

Comment 3

[Robin Mulder]

Confirming on fresh trunk install with these four extensions installed. 

Talkback ID is TB14581295M.

Note that Greasemonkey is marked incompatible with the trunk, the others are not.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Can someone post links to those extensions so that this can be easily tested?  I'm willing to try debugging this, but not to spend half a day hunting down extensions.

Comment 5

[Justin Kerk]

http://downloads.mozdev.org/ieview/ieview.xpi
http://downloads.mozdev.org/launchy/launchy.xpi
http://ftp.mozilla.org/pub/mozilla.org/extensions/greasemonkey/greasemonkey-0.6.4-fx.xpi
http://adblock.ethereal.net/SessionSaver/sessionsaver-02-dev.xpi

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Stack trace plus some debugging info

Justin, thanks for the links!

So it looks like we have a bogus entry in the wrapper table.  I'm really not sure how that can happen.  David, any ideas?  Does it matter that we're reentering GC here?

Comment 7

[Jesse Ruderman]

Bug 323371 is also a crash [@ ClassifyWrapper] and also involves Greasemonkey.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So when I crash, I see things looking like this:

(gdb) p entry->participant
$1 = (class nsIDOMGCParticipant *) 0x88e6a00
(gdb) x/wa *(void**)  entry->participant
0x897e578:      0x10

So it's dead.  During the run, I have logged:

Created wrapped native [object HTMLAnchorElement @ 0x89d3778 (native @ 0x88e6a00)], flat JSObject is 0x8593c48
Created new XPCNativeWrapper 0x8593c28 for wrapped native [object HTMLAnchorElement @ 0x89d3778 (native @ 0x88e6a00)]

which are the relevant JSObjects here.  I added some logging to the relevant Finalize hooks, and I do see them both being called.

In this case, the entry looks like:

(gdb) p *entry
$2 = {<PLDHashEntryHdr> = {keyHash = 369669479}, key = 0x89fee58, 
  keyToWrapperFunc = 0xb7545dba <HolderToWrappedJS>, participant = 0x88e6a00, next = 0x0}

so it looks like we're somehow failing to remove the preservation from an event listener...

Note that earlier in the same run I saw:

###!!! ASSERTION: nsGenericElement's event listener manager hash not empty at shutdown!: 'sEventListenerManagersHash.entryCount == 0', file ../../../../mozilla/content/base/src/nsGenericElement.cpp, line 835

So it looks like we're leaking the event listener manager and/or the content node... probably because of something the extension(s) is/are doing.  Then note that clearing the ELM hash at shutdown doesn't actually do proper clearing of the entries (on purpose), and I can see how we end up with the situation we have.

So is there any way we can deal with this gracefully?  Or just tell extensions to stop leaking?  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Some debugging stuff

Well, the first part is debugging.  The second part is a necessary correctness fix, I think.  I thought it might help here, but it didn't.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So to summarize what I _think_ happens (I can verify via more detailed logging if desired):

1)  We start shutdown.
2)  We shut down layout.  At this point, the element in question is still owned
    by its JSObject, which has not been collected because the extension is
    holding on to it somehow (dunno how).
3)  The assertion in question fires, and we finish the event listener manager
    table but don't actually call the holder destructors.  As a result, there is
    now a permanent entry for our element in the wrapper hash.
4)  Shutdown continues and at some point the extension gets a shutdown
    notification and releases stuff.
5)  Garbage collection happens and the element is destroyed after its JSObject
    is garbage collected.
6)  More garbage collection is triggered (via JSContext destruction) and we
    crash because our wrapper cache has an entry pointing to a deleted
    GCParticipant.

So to fix we either need to change what happens in step 3 or somehow make the whole shutdown thing happier here, I think.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We probably need to call elm->Disconnect() at step (3), although I'm not crazy about calling destructors then...

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possibility #1

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possibility #2

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211522 [details] [diff] [review]
possibility #2

Makes sense, I guess.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 211522 [details] [diff] [review]
possibility #2

sr=jst

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.  Let's see how this does...

Comment 17

[Peter van der Woude [:Peter6]]

(In reply to comment #16)
> Checked in to trunk.  Let's see how this does...
> 

So far so good David.
I used to 100% crash on Bug 325738 and I am unable to reproduce that now (no crash in 20 attempts)
I've asked testers on the forum to reenable the extensions that were related to this and try to crash their build.

Comment 18

[aja+bugzilla]

The crash on exit issues seem to have gone away for me too, with the most recent trunk daily: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060218 Firefox/1.6a1 ID:2006021806

Comment 19

[Stellan Klebom]

(In reply to comment #16)
> Checked in to trunk.  Let's see how this does...

As filer of this bug I can with great joy share the fact that build

  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
    Gecko/20060218 Firefox/1.6a1 ID:2006021806

no longer crashes on exit when all four of above extensions are enabled.

Thanks Boris and David!

Comment 20

[Jesse Ruderman]

*** Bug 325738 has been marked as a duplicate of this bug. ***

Comment 21

[Peter van der Woude [:Peter6]]

I filed Bug 327859 for another crash on exit case.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed on MOZILLA_1_8_BRANCH by checkin of bug 336791.