330169 - ParseNodeToXML() leaves local root stack under certain circumstances

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Michael Daumling]

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]

Attachment: 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) (inactive)]

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]]

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]

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]

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

Comment 6

[Doug Wright]

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]]

Yeah, this is a no-brainer for 1.8.0.3.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

re comment 0-> already a test available.

Comment 9

[Tim Riley [:timr]]

This needs to get landed for FF1.5.0.4.  Brendan, blake, daumling can you get this landed?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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) (inactive)]

Fixed on the 1.8 branches.

Comment 12

[Bob Clary [:bc] (inactive)]

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-