162526 - (exitcrash) Crash when quitting Mozilla [@ EventListenerManagerMapEntry::~EventListenerManagerMapEntry] [@ PL_DHashTableRawRemove] [@ 0x00000000] [@ 0x00000009 - PL_DHashTableFinish]

Status: VERIFIED FIXED

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

Description

[Brad Town]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1b) Gecko/20020812
BuildID:    2002081218

When I close Mozilla, there's a good chance (20%?) that it will report a crash
in the DDE Server Window.  This started in daily builds in the past week or two.

Reproducible: Sometimes
Steps to Reproduce:
(These are just an example of how I can get it to reproduce more often.)
1.  Restart Mozilla.  Open one browser window.
2.  Go to www.playstation.com.  Press Reload.
3.  Click "North America".  Once the page has loaded, press Reload.
4.  Click "Go there now".  Once the page has loaded, press Reload.
5.  Close the browser.  That's when I see the crash.

Actual Results:  The system reports an application error in "DDE Server Window:
mozilla.exe".

Expected Results:  The browser should close without a problem.

Talkback IDs: TB9307945X, TB9308156E; TB9294753E, TB9296121Y, TB9305259Q,
TB9306039E, TB9307528G, TB9307579M (sorry about having so many -- the first two
actually have descriptions)

Comment 1

[R.K.Aa.]

similar reports today: bug 162509, bug 162436

Comment 2

[greer]

Incident #9308156
------------------
Product ID  MozillaTrunk
Build ID 2002081218
Operating System Windows NT 5.0 build 2195
URL visited www.scea.com
User Comments Same thing as before (incident ID TB9307945X), only without
QuickLaunch.

Stack Trace
EventListenerManagerMapEntry::~EventListenerManagerMapEntry
[c:/builds/seamonkey/mozilla/content/base/src/nsGenericElement.h, line 206]
PL_DHashTableFinish [c:/builds/seamonkey/mozilla/xpcom/ds/pldhash.c, line 324]
nsGenericElement::Shutdown
[c:/builds/seamonkey/mozilla/content/base/src/nsGenericElement.cpp, line 759]
Shutdown [c:/builds/seamonkey/mozilla/content/build/nsContentModule.cpp, line 235]
nsGenericModule::Shutdown
[c:/builds/seamonkey/mozilla/xpcom/glue/nsGenericFactory.cpp, line 325]
nsDll::Shutdown [c:/builds/seamonkey/mozilla/xpcom/components/xcDll.cpp, line 394]
nsFreeLibrary
[c:/builds/seamonkey/mozilla/xpcom/components/nsNativeComponentLoader.cpp, line
309] 

Comment 3

[greer]

WFM on Win2000 on build 20020808xx.

Comment 4

[greer]

Confirming this bug as NEW. This crash is showing up in the Trunk builds from
8-12 and 8-13 exclusively in the Talkback data.

-> topcrash, zt4newcrash.

looks like jst touched this on the 12th. cc'ing him.

Comment 5

[greer]

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

Comment 6

[greer]

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

Comment 7

[greer]

TO -> XPCOM (per duped bug 162436), adding cc's, url and signatures from that bug.
OS -> ALL

Comment 8

[greer]

reassinging to dougt (only because changing components to match bug 162436
didn't sync up the bug assignment). 

This might be johnny's bug based on the more fully resolved stack in comment #2
and the cvs log (comment #4).

Comment 9

[ly nguyen]

since this bug is zt4newcrash, marking it as blocker

Comment 10

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

mkaply was saying this was from jst's changes.  In any case, dougt certainly
shouldn't own this if only pldhash is implicated, but it's unlikely to be
pldhash rather than the user of pldhash...

mkaply was also saying that this happens on OS/2 after visiting pages with Java
on them.

Comment 11

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

Attachment: a fix for one potential problem

This fixes one potential problem.

Comment 12

[Johnny Stenback (:jst)]

That really *shouldn't* matter, the typo's should be fixed, but the memset
really should *not* be needed. I'd rather not take that fix unless someone can
show that it actually fixes the crash, and why.

Comment 13

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

Oh, right, never mind.

/me remembers that that's only there to maintain the invariant that all new
pldhash entries are zero-initialized by the default ops...

Comment 14

[jhp (no longer active)]

This doesn't fix the crash for me on OS/2 (haven't tried windows).  I get the
crash consistently by opening a page with a java applet and then closing the
browser (http://www.kxas.com in my case).  I was debugging this, and it seems
that EventListenerManagerHashClearEntry is getting a bogus entry.  It gets cast,
and in the destructor for EventListenerManagerMapEntry tries to call
mListenerManager->SetListenerTarget(nsnull), which sends control to an incorrect
memory location, causing the crash.

Comment 15

[Johnny Stenback (:jst)]

I just checked in the typo fixes from dbaron's patch.

Comment 16

[Andrew Schultz]

I seem to be hitting this bug with linux build 20020813.
1. Go to http://www.heathrowexpress.co.uk.
2. Close the main window then the popup.

Stacktraces were rarely the same.  Backing out bug 156364 fixed the crash.

Comment 17

[Mike Kaply [:mkaply]]

Why isn't this following the zt4newcrash policy?

There seems to be no work on it, yet it is clearly a regression and a crash.

Comment 18

[Andrew Schultz]

Attachment: testcase popup

Comment 19

[Andrew Schultz]

Attachment: testcase (parent)

open the testcase (this attachment) in a new browser session.
close the parent window.
close the popup

crashes linux build 20020813 every time

Comment 20

[hhwong]

Well, the patch that (I assume) is now in 2002081504 seems to be a little
better, but still crashing once in a while instead of everytime I closed the
browser as it did with yesterday's build. With the testcases (and playing with
the Orbitz website because of a different bug), is it pop-up related now?

Talkback crash: TB9396979Y (not that you need any more of these I'm guessing)

Comment 21

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

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

Comment 22

[Brendan Eich [:brendan]]

Is QuickStart implicated?

/be

Comment 23

[hhwong]

I'm not using Quickstart....

Comment 24

[Jay Patel [:jay]]

Adding [@ PL_DHashTableRawRemove] to summary from duped bug 162695 for tracking.

So do we have a fix for this crash (was the checkin mentioned in comment #15 a
patch to fix the crash?) or do we have to back out jst's checkin for  bug
156364?  Just wondering since this is a zt4newcrash bug.

Comment 25

[Johnny Stenback (:jst)]

I can not for the life of me reproduce this...

Comment 26

[Andrew Schultz]

have you tried the testcase / heathrowexpress on Linux (perhaps it works ok on
Windows)?  It crashes every time for me.  None of the other cases crash for me
under Linux.

Comment 27

[Johnny Stenback (:jst)]

Yes, I've tried on linux, no crash here.

Comment 28

[Johnny Stenback (:jst)]

The one thing that I can think of here that could cause crashes or whatnot is if
we end up re-entering code that uses these new hashes *while* the hashes are
being torn down. We could fix this by doing something like this:

  PLDHashTable rangeListsHash = sRangeListsHash;
  PLDHashTable eventListenerManagersHash = sEventListenerManagersHash;

  sRangeListsHash.ops = nsnull;
  sEventListenerManagersHash.ops = nsnull;

  PL_DHashTableFinish(&rangeListsHash);
  PL_DHashTableFinish(&eventListenerManagersHash);

David, whaddayathink about this, is it safe to rely on tranfering a PLDHashTable
into a new struct like that?

Comment 29

[Andrew Schultz]

Attachment: valgrind log for the testcase

this is the output from valgrind loading the testcase.	I can run again with
more frames in the stacks if it would be useful.

The behavior shown here is consistent with the fact that my stacks (with a
debug build) always end up in a free().

Comment 30

[Johnny Stenback (:jst)]

If the problem here turns out to be that things are now released too late where
those things were just not released earlier then we could of course simply
disable the proper cleanup we're now doing on shutdown which would bring us back
to where we were before I checked in the fix for bug 156364. If we can't think
of anything else that's always one way back w/o backing out the fix for bug
156364 which I *really* *really* *really* *really* don't want to do.

Comment 31

[Kai Lahmann (is there, where MNG is)]

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

Comment 32

[Kai Lahmann (is there, where MNG is)]

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

Comment 33

[Doug Turner (:dougt)]

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

Comment 34

[timeless]

72hrs.

Comment 35

[Johnny Stenback (:jst)]

Attachment: Maybe this would cure this crash?

This patch makes us leak what we used to leak before I landed my changes that
caused this crash, and thus this prevents the code that causes the crash from
being executed during shutdown. I'll leave the asserts in there that shows when
we've leaked nsGenericElement's.

Comment 36

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

Comment on attachment 95661 [details] [diff] [review]
Maybe this would cure this crash?

sr=dbaron, although it might be nice to use a separate set of hashtable ops
(since I'd think it affects the ability to restart XPCOM and the layout engine
within the same process, since we don't actually unload the DLLs)

Comment 37

[Johnny Stenback (:jst)]

Attachment: Same as above, but don't mess with the static ops structs.

Comment 38

[Johnny Stenback (:jst)]

Comment on attachment 95663 [details] [diff] [review]
Same as above, but don't mess with the static ops structs.

Carrying sr=dbaron forward.

Comment 39

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

Comment on attachment 95663 [details] [diff] [review]
Same as above, but don't mess with the static ops structs.

sr=dbaron

Comment 40

[John Keiser (jkeiser)]

Comment on attachment 95663 [details] [diff] [review]
Same as above, but don't mess with the static ops structs.

r=jkeiser

Comment 41

[Andrew Schultz]

CVS this morning does not crash on the testcase, but the URL (heathrowexpress)
still does.  The patch prevents the crash on the URL

The assertion appears with and without the patch.
"nsGenericElement's event listener manager hash not empty at shutdown!"

Comment 42

[Johnny Stenback (:jst)]

Fix checked in, marking FIXED, please reopen if this crash still shows up.

Comment 43

[Johnny Stenback (:jst)]

Yes, that assertion would appear if we've leaked nsGenericElement's, which we do
on those pages. Fixing those leaks would've fixed this crash also, but finding
those leaks is non-trivial, and I bet there's more than one place where we leak
those elements so fixing those leaks is not a guarantee that this crash won't
show up again later on some other sites.

Comment 44

[Wesha]

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

Comment 45

[Wesha]

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

Comment 46

[Matthias Versen [:Matti]]

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

Comment 47

[alanjstr]

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

Comment 48

[Wesha]

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

Comment 49

[av (gone)]

Johnny, please look at bug 164386 -- could be an incarnation of this one.

Comment 50

[Jay Patel [:jay]]

The crash originally reported under the
EventListenerManagerMapEntry::~EventListenerManagerMapEntry stack signature is
no longer showing up in Talkback data.  Last crash reported was with 8/16
MozillaTrunk...so marking this verified.  

Bug 164386 has been opened for current PL_DHashTableRawRemove crashes.