52859 - a case where js_AllocGCThing asserts

Status: RESOLVED WONTFIX

(Core :: Layout, defect, P2)

Description

[John Bandhauer]

Here's the stack of a case where js_AllocGCThing asserts because a finalizer is 
in the stack. Sorry, I did this the other and don't rememeber exactly what I 
did. It looks like closing a window was part of it.

Comment 1

[John Bandhauer]

Attachment: stack shhowing assert location

Comment 2

[Brendan Eich [:brendan]]

Should I reassign to whomever owns the rat's-nest that leads to this undesirable
outcome?  It's undesirable for the DOM/embedding code too, not just for the new
JS GC -- why are we doing all that re-setup after tearing things down?  I would
like to stick to my incompatible guns.  Thoughts?

What happens if you continue through the assert?

/be

Comment 3

[Brendan Eich [:brendan]]

nsBrowserInstance::ReinitializeContentVariables() line 425 + 42 bytes
nsBrowserInstance::GetContentAreaDocShell(nsIDocShell * * 0x0012d3f4) line 447
nsBrowserInstance::Close(nsBrowserInstance * const 0x03402b10) line 1443 + 32
bytes
nsBrowserInstance::~nsBrowserInstance() line 410

Argh!  nsBrowserInstance.cpp!  So it seems truly bogus for a destructor to cause
reinitialization and a new script object to be allocated.  Cc'ing hyatt for some
advice.  After Netscape 6, we should land the rest of shaver's extensions/wallet
evisceration and then get rid of nsBrowserInstance.cpp altogether.

/be 

Comment 4

[Brendan Eich [:brendan]]

I think this bug essentially dups the bug or bugs about
nsBrowserInstance::ReinitializeContentVariables (bug 46454 and bug 53953 at
least).  It's not mine to fix.  Reassigning to don to find an nsBrowserInstance
owner who can fix.  Danm thought radha might be working on such a bug, to make
skin switching safe.

/be

Comment 5

[Dan M]

  In particular, the problem happens because of some code Hyatt wrote that 
teaches nsBrowserInstance to reinitialize itself at any time it's asked to do 
something, and it notices that its docshell has been pulled out from underneath 
it. This code was added because skin switching did just that: it destroyed all 
frames, and the docshell.
  A side effect of giving nsBrowserInstance such power to reinitialize itself 
under any circumstances is that it's doing it in bad places, like when the window 
is being shut down. It thinks it can, but it really can't. Many subtle and ugly 
bugs result.
  Now I know Radha was working on a notification system; some function of 
nsBrowserInstance that the skin switching code could call at the time the switch 
was made, telling nsBrowserInstance to reinitialize itself right then, and only 
then. I can't find the bug describing that effort, if there was one. It was a 
desirable thing for several similar reasons; bugs caused by nsBrowserInstance's 
unseemly ability to reconstitute itself.
  If she was successful in doing this, then the Unseemly Ability could be ripped 
out. nsBrowserInstance would only do this at carefully controlled times, not when 
it's being destroyed. With the UA ripped out, a batch of subtle bugs, like this 
one, would go away.

Comment 6

[don]

Radha, do you know about this?  Are you the right owner here?

Comment 7

[Radha on family leave (not reading bugmail)]

I haven't done anything to get  notifications in BrowserInstance whenever skin 
is swiched. I understand that such a notificaiton should come from the 
layout(presShell). Need to talk to layouyt folks. Post 6.0

Comment 8

[Radha on family leave (not reading bugmail)]

Alec is working on getting rid of browser Instance. I think he can handle this. 

Comment 9

[Alec Flett]

yeehaw.. die browserinstance, die!

Comment 10

[[e-mail] (gone - use [e-mail] instead)]

Adding Seth and myself to this bug.

Comment 11

[John Bandhauer]

Note that my fix for a similar "js_AllocGCThink during gc" problem in bug 83367
included changing xpconnect to defer Release calls on wrapped natives until gc
is finished. This means that the Release call - and eventual dtor invocation -
is no longer done during gc. So this exact problem should not happen anymore.
But that don't mean this nsBrowserInstance shouldn't be cleaned up :)

Comment 12

[Phil Schwartau]

Updating component and QA to a browser area, as this is not JS Engine 
per se. Based on the discussion above, choosing Layout. Not changing 
current owner (alecf). 

Comment 13

[Paul Chen]

nav triage team;

If jband's last comment is correct, this seems to be a proxy for alecf to fix
nsBrowserInstance (or get rid of it all together?). Pushing out to mozilla1.2

Comment 14

[Alec Flett]

ok, nobody knows what caused this, we haven't seen this in ages, and I don't
think that trying to fix this will actually make nsBrowserInstance go away.
Marking WONTFIX