we unroot nsXULPrototypeScript that were never rooted

RESOLVED FIXED in mozilla0.9.4

Status

()

defect
P1
major
RESOLVED FIXED
18 years ago
2 months ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({memory-leak})

Trunk
mozilla0.9.4
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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...
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).
Brendan caused this in the fastload landing.  I have one idea for how to fix it,
but it may not be the best idea.
Assignee: jst → brendan

Updated

18 years ago
Keywords: mlk
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.)
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=brendan@mozilla.org.  Hey
waterson, care to sr=?

/be
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.4
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
ok, taking
Assignee: brendan → dbaron
Status: ASSIGNED → NEW

Comment 8

18 years ago
sr=waterson
Fix checked in 2001-08-01 19:07 PDT.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.