362110 - jsscope property sweeping still calls malloc in some cases

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Gavin Reaney]

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]

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

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

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

Missed 1.8.1.1, moving request out.

Comment 5

[Daniel Veditz [:dveditz]]

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

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

Can I have a description of how to test to verify that this bug is fixed?

Comment 8

[Gavin Reaney]

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

Attachment: 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.