298459 - Crash @gc_root_marker (JS_ASSERT)

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

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

I'm hitting the JS_ASSERT at jsgc.c:1474
JS_ASSERT(root_points_to_gcArenaPool);

The console shows:
JS API usage error: the address passed to JS_AddNamedRoot currently holds an
invalid jsval.  
This is usually caused by a missing call to JS_RemoveRoot.
The root's name is "WindowStateHolder::mJSObj".

I first hit this with 20050616 or 0618, and did not notice it in 0613.

browser.sessionhistory.max_viewers is 5.

I don't know steps to reproduce.

Comment 1

[Brendan Eich [:brendan]]

We talked about this on IRC, remember?  It's an API user error, not a JS engine
bug, when you see this assertion.

Looking at nsGlobalWindow.cpp, though, I don't see how the WindowStateHolder
dtor can not be called, yet the memory go free (or really, mJSObj become
invalid).  Cc'ing bryner.

Since this is suite, is there any code not checked in that is in your build?  We
need to see all the sources.

/be

Comment 2

[Brendan Eich [:brendan]]

Maybe an allocation and then a GC (or two GCs) are nesting in the ctor.  I said
to bryner on IRC that I should have noticed during review the lateness of the
call to JS_AddNamedRoot (the last thing the ctor does).  Moving that up to be
first may just fix this bug, although it would be even better to understand how
the mJSObj root comes to dangle.

/be

Comment 3

[Brian Ryner (not reading)]

Moved AddNamedRoot earlier in WindowStateHolder's ctor after talking this over
with brendan.

Checking in nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.746; previous revision: 1.745
done

marking fixed.

Comment 4

[Brendan Eich [:brendan]]

Just had this happen.  I'm running an optimized trunk build.  Is there a missing
virtual dtor or something?

/be

Comment 5

[Brendan Eich [:brendan]]

THis is real, it's still happening.  Need to figure out why.  Theories,
speculation welcome.

/be

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 9

[Brian Ryner (not reading)]

Attachment: fix

Something on this page is making us unhappy, and among other things we hit the
case where we fail to copy a property.	The cleanup deletes the state object as
an nsISupports*, which fails to run the WindowStateHolder dtor.  This fixes
that by using Release() via an nsCOMPtr.

Comment 10

[Darin Fisher]

Comment on attachment 192038 [details] [diff] [review]
fix

r=darin

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 192038 [details] [diff] [review]
fix

requesting approval, this is a dead-simple fix

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 192038 [details] [diff] [review]
fix

I should have seen this, it was just what it should have been to cause the
symptom.

/be

Comment 13

[Brian Ryner (not reading)]

checked in

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

this might have caused bug 304003

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

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