jsscope property sweeping still calls malloc in some cases

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P3
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Gavin Reaney, Assigned: Gavin Reaney)

Tracking

({fixed1.8.1.2})

Trunk
mozilla1.9alpha1
fixed1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [needs testcase])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

11 years ago
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.
Attachment #246809 - Flags: review?(brendan)

Updated

11 years ago
Blocks: 355044
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
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
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
Last Resolved: 11 years ago
Flags: blocking1.8.1.1?
Resolution: --- → FIXED

Updated

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

Comment 8

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

Comment 9

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