Last Comment Bug 713069 - Remove AutoEnterCompartment calls from finalizers and assert
: Remove AutoEnterCompartment calls from finalizers and assert
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
Depends on: 717173
Blocks: 712480
  Show dependency treegraph
Reported: 2011-12-22 12:32 PST by Bill McCloskey (:billm)
Modified: 2012-01-11 08:08 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch, v1 (6.16 KB, patch)
2011-12-27 14:37 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
mrbkap: review+
Details | Diff | Review
Patch, v1.1 (6.37 KB, patch)
2011-12-28 11:09 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Review

Description Bill McCloskey (:billm) 2011-12-22 12:32:24 PST
For bug 712480, it would be nice to be able to assert that we don't use AutoEnterCompartment from within a finalizer. Entering a compartment is often a prelude to running JS code, and that's what we want to catch.

Basically, it looks like there are a few of these calls that happen with the following stack:

 1!CrashInJS [jsutil.cpp : 98 + 0xb]
    eip = 0x024b190e   esp = 0xbf8abc20   ebp = 0xbf8abc38
    Found by: previous frame's frame pointer
 2!JSAutoEnterCompartment::enter [jsapi.cpp : 241 + 0x2b]
    eip = 0x02300cf6   esp = 0xbf8abc40   ebp = 0xbf8abc88   ebx = 0x02b71878
    Found by: call frame info
 3!mozilla::dom::workers::WorkerPrivateParent<mozilla::dom::workers::WorkerPrivate>::FinalizeInstance [WorkerPrivate.cpp : 1886 + 0x7]
    eip = 0x0197f4ca   esp = 0xbf8abc90   ebp = 0xbf8abd08   ebx = 0x02b71878
    esi = 0x0a6f8088   edi = 0x09011b20
    Found by: call frame info
 4!Worker::Finalize [Worker.cpp : 247 + 0x9]
    eip = 0x019770a6   esp = 0xbf8abd10   ebp = 0xbf8abd38   ebx = 0x02b71878
    esi = 0x09011b20   edi = 0xa06bc700
    Found by: call frame info
 5!js::gc::Arena::finalize<JSObject> [jsobjinlines.h : 279 + 0xb]
    eip = 0x023a5d2c   esp = 0xbf8abd40   ebp = 0xbf8abdc8   ebx = 0x02b71878
    esi = 0xa06bc700   edi = 0xa06bc6c0
    Found by: call frame info well as a call from Notify, which happens within the worker finalizer. The goal here is to avoid AutoEnterCompartment during finalization and add an assert to JSAutoEnterCompartment::enter that we're not in a GC.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-27 14:37:50 PST
Created attachment 584490 [details] [diff] [review]
Patch, v1

This should make it so we don't enter a compartment from the finalizer.

Bill, can you try this patch and test your assertion again?
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-12-28 06:47:41 PST
Comment on attachment 584490 [details] [diff] [review]
Patch, v1

>   bool
>   PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
>   {
>     // Modify here, but not in PostRun! This busy count addition will be matched
>-    // by the CloseEventRunnable.
>-    return aWorkerPrivate->ModifyBusyCount(aCx, true);
>+    // by the CloseEventRunnable. Only do this if we're not running from the
>+    // finalizer.
>+    return mFromJSObjectFinalizer ?
>+           true :
>+           aWorkerPrivate->ModifyBusyCount(aCx, true);

The second hunk of this comment isn't terribly useful since it simply parrots the code. Can you instead explain why it isn't necessary to balance the CloseEventRunnable?

My understanding is that because the worker is being finalized, we know that we don't have any more work to do, so there won't be a CloseEventRunnable. Is that right?
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-28 11:09:32 PST
Created attachment 584601 [details] [diff] [review]
Patch, v1.1

Fixed the comment, and fixed one additional thing (forgot to use a null cx).
Comment 4 Bill McCloskey (:billm) 2011-12-28 13:29:53 PST
The modified patch fixes the assertion I was getting.
Comment 5 Bill McCloskey (:billm) 2011-12-30 16:35:02 PST
Is this ready to land?
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-31 01:08:43 PST
Comment 7 Phil Ringnalda (:philor) 2011-12-31 19:07:30 PST

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