Last Comment Bug 362110 - jsscope property sweeping still calls malloc in some cases
: jsscope property sweeping still calls malloc in some cases
Status: RESOLVED FIXED
[needs testcase]
: fixed1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla1.9alpha1
Assigned To: Gavin Reaney
:
Mentors:
Depends on:
Blocks: js1.7src
  Show dependency treegraph
 
Reported: 2006-11-28 08:46 PST by Gavin Reaney
Modified: 2007-02-02 15:28 PST (History)
8 users (show)
dveditz: blocking1.8.1.2+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prevent mallocs when sweeping (4.41 KB, patch)
2006-11-28 09:46 PST, Gavin Reaney
no flags Details | Diff | Splinter Review
fix with comment and vertical space nits picked (7.92 KB, patch)
2006-11-29 13:40 PST, Brendan Eich [:brendan]
brendan: review+
dveditz: approval1.8.1.2+
Details | Diff | Splinter Review
Patch to help verify the bug fix (1.41 KB, patch)
2007-02-02 15:22 PST, Gavin Reaney
no flags Details | Diff | Splinter Review
Test case (1.09 KB, application/x-javascript)
2007-02-02 15:28 PST, Gavin Reaney
no flags Details

Description Gavin Reaney 2006-11-28 08:46:29 PST
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.
Comment 1 Gavin Reaney 2006-11-28 09:46:47 PST
Created attachment 246809 [details] [diff] [review]
Prevent mallocs when sweeping

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.
Comment 2 Brendan Eich [:brendan] 2006-11-29 13:40:41 PST
Created attachment 246981 [details] [diff] [review]
fix with comment and vertical space nits picked

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
Comment 3 Brendan Eich [:brendan] 2006-12-02 15:13:18 PST
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
Comment 4 Daniel Veditz [:dveditz] 2006-12-04 10:42:22 PST
Missed 1.8.1.1, moving request out.
Comment 5 Daniel Veditz [:dveditz] 2007-01-05 15:08:18 PST
Comment on attachment 246981 [details] [diff] [review]
fix with comment and vertical space nits picked

approved for 1.8 branch, a=dveditz for drivers
Comment 6 Brendan Eich [:brendan] 2007-01-05 15:17:58 PST
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
Comment 7 alice nodelman [:alice] [:anode] 2007-01-30 15:46:56 PST
Can I have a description of how to test to verify that this bug is fixed?
Comment 8 Gavin Reaney 2007-02-02 15:22:46 PST
Created attachment 253794 [details] [diff] [review]
Patch to help verify the bug fix

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.
Comment 9 Gavin Reaney 2007-02-02 15:28:22 PST
Created attachment 253796 [details]
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.

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