Last Comment Bug 330169 - ParseNodeToXML() leaves local root stack under certain circumstances
: ParseNodeToXML() leaves local root stack under certain circumstances
Status: RESOLVED FIXED
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla1.9alpha1
Assigned To: Michael Daumling
:
: Jason Orendorff [:jorendorff]
Mentors:
: 330857 (view as bug list)
Depends on:
Blocks: js1.6rc1
  Show dependency treegraph
 
Reported: 2006-03-11 07:11 PST by Michael Daumling
Modified: 2006-07-02 14:05 PDT (History)
7 users (show)
brendan: blocking1.8.1+
dveditz: blocking1.8.0.4+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Goto end of function and leave correctly. (2.12 KB, patch)
2006-03-11 07:29 PST, Michael Daumling
mrbkap: review+
brendan: superreview+
mrbkap: approval‑branch‑1.8.1+
Details | Diff | Splinter Review

Description Michael Daumling 2006-03-11 07:11:30 PST
When testing /c/mozilla/js/tests/e4x/Expressions/11.1.4-04.js, I discovered that ParseNodeToXML() has two early exits like this:

return PN2X_SKIP_CHILD;

without calling js_LeaveLocalRootScopeWithResult(). This leaves the local root stack created during function entry active. When doing a for...each loop, this leads to the entire global object still being rooted (in jsinterp.c, the PropertyIterator's parent is rooted) - causing a massive memory leak, especially if multiple global objects are used.
Comment 1 Michael Daumling 2006-03-11 07:29:31 PST
Created attachment 214780 [details] [diff] [review]
Goto end of function and leave correctly.

Instead of returning early, jump to the end of the function, call js_LeaveLocalRootScope() and then return.
Comment 2 Blake Kaplan (:mrbkap) 2006-03-11 11:39:37 PST
Comment on attachment 214780 [details] [diff] [review]
Goto end of function and leave correctly.

I can't find any obvious way to combine the two common return paths, so r=mrbkap.
Comment 3 Brendan Eich [:brendan] 2006-03-11 13:12:18 PST
Comment on attachment 214780 [details] [diff] [review]
Goto end of function and leave correctly.

>+skipChild:
>+    js_LeaveLocalRootScope(cx);
>+    return PN2X_SKIP_CHILD;
>+

Esoteric code style rule (see bug 328896 comment 2) favors skip_child style for label names.

Thanks for finding and fixing this!

/be
Comment 4 Michael Daumling 2006-03-15 01:26:32 PST
It took me quite a fwhile to track this one down :)

Changed label name from skipChild to ckip_skild and committed the fix (3.92).
Comment 5 Doug Wright 2006-03-17 12:26:30 PST
*** Bug 330857 has been marked as a duplicate of this bug. ***
Comment 6 Doug Wright 2006-03-17 12:34:18 PST
To whoever evaluates this for inclusion into 1.8.0.3: please see bug 330857 (dupe of this) for the real-world effect this bug is having.
Comment 7 Brendan Eich [:brendan] 2006-03-17 15:08:38 PST
Yeah, this is a no-brainer for 1.8.0.3.

/be
Comment 8 Bob Clary [:bc:] 2006-03-20 12:01:16 PST
re comment 0-> already a test available.
Comment 9 Tim Riley [:timr] 2006-04-28 12:29:20 PDT
This needs to get landed for FF1.5.0.4.  Brendan, blake, daumling can you get this landed?
Comment 10 Blake Kaplan (:mrbkap) 2006-04-29 20:28:23 PDT
Comment on attachment 214780 [details] [diff] [review]
Goto end of function and leave correctly.

I *think* I'm allowed to do this -- it's approved for 1.8.0 anyway!
Comment 11 Blake Kaplan (:mrbkap) 2006-04-29 20:30:16 PDT
Fixed on the 1.8 branches.
Comment 12 Bob Clary [:bc:] 2006-07-02 14:05:18 PDT
Even though this bug was discovered with an existing test case, the actual test does not cover the particulars of this bug. For that reason -> in-testsuite-

Note You need to log in before you can comment on or make changes to this bug.