The default bug view has changed. See this FAQ.

ParseNodeToXML() leaves local root stack under certain circumstances

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Michael Daumling, Assigned: Michael Daumling)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
mozilla1.9alpha1
fixed1.8.0.4, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.4 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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.
Attachment #214780 - Flags: review?(mrbkap)
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.
Attachment #214780 - Flags: review?(mrbkap) → review+
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
Attachment #214780 - Flags: superreview+

Updated

11 years ago
Assignee: general → daumling
Blocks: 309169
Flags: blocking1.8.1+
Flags: blocking1.8.0.3?
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 4

11 years ago
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).
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 5

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

Comment 6

11 years ago
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.
Yeah, this is a no-brainer for 1.8.0.3.

/be

Comment 8

11 years ago
re comment 0-> already a test available.
Flags: in-testsuite+
Flags: blocking1.8.0.3? → blocking1.8.0.3+

Comment 9

11 years ago
This needs to get landed for FF1.5.0.4.  Brendan, blake, daumling can you get this landed?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 years ago
Assignee: daumling → mrbkap
Status: REOPENED → NEW
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!
Attachment #214780 - Flags: approval-branch-1.8.1+

Updated

11 years ago
Assignee: mrbkap → daumling
Fixed on the 1.8 branches.
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Keywords: fixed1.8.0.4, fixed1.8.1
Resolution: --- → FIXED

Comment 12

11 years ago
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-
Flags: in-testsuite+ → in-testsuite-
You need to log in before you can comment on or make changes to this bug.