93146 - we unroot nsXULPrototypeScript that were never rooted

Status: RESOLVED FIXED

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

Description

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

I've been investigating why we occasionally leak nsXULPrototypeScript::mJSObject
and sometimes nsXULPrototypeAttribute::mEventHandler JS roots.  The problem is
that we call RemoveJSGCRoot more than we call AddJSGCRoot (these are in
nsXULElement.cpp).  When we do this, the gJSRuntime gets set to null early, and
we fail to remove the same number of roots as there were excess calls, except
the ones we don't remove are the ones called last.  This means we leak real roots.

I haven't figured out the cause of the imbalance yet, but I'm still looking...

Comment 1

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

XULContentSinkImpl::OpenScript and nsXULDocument::LoadScript seem to be filling
in the mJSObject of an nsXULPrototypeScript without rooting it (presumably
because it's already in the XUL cache and thus rooted).

Comment 2

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

Brendan caused this in the fastload landing.  I have one idea for how to fix it,
but it may not be the best idea.

Comment 3

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

Attachment: possible fix

Comment 4

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

This fix shouldn't cause too much extra work, I think, since all
nsXULPrototypeScript eventually get an mJSObject.  Presumably that happens
pretty quickly, so there won't be many (if any) GC runs before they do (that
would cause an extra call to gc_root_marker that would break out since v is
null).  But maybe there's a better way.  (And I wonder if we have similar
problems for nsXULPrototypeAttribute::mEventHandler (maybe bug 87040?), where a
fix like this one *would* be a big performance cost.)

Comment 5

[Brendan Eich [:brendan]]

dbaron: I like your patch; the redundant roots don't cost much (e.g., for
strres.js when it's hit in the XUL script cache) and it all works well even in
the face of imbalance leading to too many Removes.  r=[e-mail].  Hey
waterson, care to sr=?

/be

Comment 6

[Brendan Eich [:brendan]]

Thanks again to dbaron for tracking this down, patching, and offering to do the
checkin.  Cc'ing shaver for sr= cuz waterson's playing b-ball.

/be

Comment 7

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

ok, taking

Comment 8

[Chris Waterson]

sr=waterson

Comment 9

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

Fix checked in 2001-08-01 19:07 PDT.