Bug 88355 - Entry into JSC should CRASH() if the Heap is busy
Summary: Entry into JSC should CRASH() if the Heap is busy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-05 12:18 PDT by Mark Hahnenberg
Modified: 2012-06-06 13:32 PDT (History)
1 user (show)

See Also:


Attachments
Patch (5.96 KB, patch)
2012-06-05 13:03 PDT, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2012-06-05 12:18:44 PDT
Interpreter::execute() returns jsNull() right now if we try to enter it while the Heap is busy (e.g. with a collection), which is okay, but some code paths that call Interpreter::execute() allocate objects before checking if the Heap is busy. Attempting to execute JS code while the Heap is busy should not be allowed and should be enforced by a release-mode CRASH() to prevent vague, unhelpful backtraces later on if somebody makes a mistake. Normally, recursively executing JS code is okay, e.g. for evals, but it should not occur during a Heap allocation or collection because the Heap is not guaranteed to be in a consistent state (especially during collections). We are protected from executing JS on the same Heap concurrently on two separate threads because they must each take a JSLock first. However, we are not protected from reentrant execution of JS on the same thread because JSLock allows reentrancy. Therefore, we should fail early if we detect an entrance into JS code while the Heap is busy.
Comment 1 Mark Hahnenberg 2012-06-05 13:03:25 PDT
Created attachment 145857 [details]
Patch
Comment 2 Geoffrey Garen 2012-06-05 13:14:57 PDT
Comment on attachment 145857 [details]
Patch

r=me

Can we put one of these assertions into the allocation slow path as well?
Comment 3 Mark Hahnenberg 2012-06-05 13:20:15 PDT
> Can we put one of these assertions into the allocation slow path as well?

Will do.
Comment 4 Mark Hahnenberg 2012-06-05 13:38:37 PDT
Committed r119518: <http://trac.webkit.org/changeset/119518>
Comment 5 Geoffrey Garen 2012-06-06 13:32:05 PDT
>     ASSERT(!m_heap->isBusy()); 

Actually, I had a CRASH() on the allocation slow path in mind.