713069 - Remove AutoEnterCompartment calls from finalizers and assert

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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  libxul.so!CrashInJS [jsutil.cpp : 98 + 0xb]
    eip = 0x024b190e   esp = 0xbf8abc20   ebp = 0xbf8abc38
    Found by: previous frame's frame pointer
 2  libxul.so!JSAutoEnterCompartment::enter [jsapi.cpp : 241 + 0x2b]
    eip = 0x02300cf6   esp = 0xbf8abc40   ebp = 0xbf8abc88   ebx = 0x02b71878
    Found by: call frame info
 3  libxul.so!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  libxul.so!Worker::Finalize [Worker.cpp : 247 + 0x9]
    eip = 0x019770a6   esp = 0xbf8abd10   ebp = 0xbf8abd38   ebx = 0x02b71878
    esi = 0x09011b20   edi = 0xa06bc700
    Found by: call frame info
 5  libxul.so!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

...as 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!)]

Attachment: 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) (inactive)]

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

Attachment: Patch, v1.1

Fixed the comment, and fixed one additional thing (forgot to use a null cx).

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The modified patch fixes the assertion I was getting.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Is this ready to land?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a895e9432ea4

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a895e9432ea4