Closed Bug 362110 Opened 15 years ago Closed 15 years ago

jsscope property sweeping still calls malloc in some cases

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: gavin.reaney, Assigned: gavin.reaney)

References

Details

(Keywords: fixed1.8.1.2, Whiteboard: [needs testcase])

Attachments

(3 files, 1 obsolete file)

Bug 357388 fixed some crashes that can occur in low memory conditions if malloc fails when sweeping the property scope tree.

The basis of that fix was to remove the need to call malloc, and it is correct as far as it goes. However, there are two cases which were missed where we can still end up trying to malloc a new KidsChunk. This can result in an assertion failure (although not a crash as far as I can tell).

The asserts fail at jsscope.c line 1609 or 1627 (version 3.49). The cases which need addressed for js_SweepScopeProperties are:

1) If we have a grandparent with no kids and we try to insert more than one kid into it this causes a new KidsChunk to be allocated.

2) If we have a grandparent with 11 kids, and we remove one, this leaves an unused KidsChunk which is free'd by RemovePropertyTreeChild (since each KidsChunk holds 10 kids). This can result in a new KidsChunk needing to be allocated when we next call InsertPropertyTreeChild.

Both issues are relatively easy to fix:

For 1 we need to update InsertPropertyTreeChild to reuse the 'sweptChunk' that is passed in to the function in the branch of code that deals with non-chunky kids.

For 2 we need to reuse the chunk that is now available when RemovePropertyTreeChild completes. The simplest way to do this is to return it to the caller and pass it into InsertPropertyTreeChild as required.

Patch to follow.
Attached patch Prevent mallocs when sweeping (obsolete) — Splinter Review
This fixes the issues described previously.

Tested by checking that NewPropertyTreeChild is now never called during sweeping (www.ebay.com is one site which would previously cause this to happen). The assertions can no longer fire in OOM situations as a result.

Brendan, can you review please? Sorry for missing these cases first time around.
Attachment #246809 - Flags: review?(brendan)
Blocks: js1.7src
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Thanks, I should have seen this too.  My spider-sense was tingling and I foolishly ignored it.  Trying to get this into the trunk ASAP so it can ride into 1.8.1.1.

/be
Attachment #246809 - Attachment is obsolete: true
Attachment #246981 - Flags: review+
Attachment #246809 - Flags: review?(brendan)
Status: NEW → ASSIGNED
Whiteboard: [checkin needed]
Fixed on trunk:

Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.50; previous revision: 3.49
done

/be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: blocking1.8.1.1?
Resolution: --- → FIXED
Flags: in-testsuite-
Missed 1.8.1.1, moving request out.
Flags: blocking1.8.1.1? → wanted1.8.1.x?
Whiteboard: [checkin needed]
Flags: wanted1.8.1.x? → blocking1.8.1.2+
Comment on attachment 246981 [details] [diff] [review]
fix with comment and vertical space nits picked

approved for 1.8 branch, a=dveditz for drivers
Attachment #246981 - Flags: approval1.8.1.2+
Fixed on the 1.8 branch:

Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.45.20.5; previous revision: 3.45.20.4
done

/be
Keywords: fixed1.8.1.2
Can I have a description of how to test to verify that this bug is fixed?
Whiteboard: [needs testcase]
This patch can be applied to check that no property tree mallocs are attempted when sweeping the property scope tree (if a malloc is attempted an assert will fail). You can use this with a build of the browser or the standalone JS shell (a debug build is required). I'll attach a small test case next.
Attached file Test case
This test case checks the two scenarios described in comment 0. When attachment 253794 [details] [diff] [review] is applied the test case should complete without any asserts firing. Prior to this bug fix the asserts do fire.
You need to log in before you can comment on or make changes to this bug.