Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove AutoEnterCompartment calls from finalizers and assert

RESOLVED FIXED in mozilla12



6 years ago
6 years ago


(Reporter: billm, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



6 years ago
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.
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?
Attachment #584490 - Flags: review?(mrbkap)
Attachment #584490 - Flags: review?(jonas)
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?
Attachment #584490 - Flags: review?(mrbkap) → review+
Created attachment 584601 [details] [diff] [review]
Patch, v1.1

Fixed the comment, and fixed one additional thing (forgot to use a null cx).
Attachment #584490 - Attachment is obsolete: true
Attachment #584490 - Flags: review?(jonas)
Attachment #584601 - Flags: review+

Comment 4

6 years ago
The modified patch fixes the assertion I was getting.

Comment 5

6 years ago
Is this ready to land?
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Version: unspecified → Trunk
Depends on: 717173
You need to log in before you can comment on or make changes to this bug.