TM: nsDOMWorker has to set the right compartment

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: gwagner, Assigned: jorendorff)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
0x00000001018fb4fc in JS_Assert (s=0x101d2cf70 "(thingKind == js::gc::FINALIZE_STRING) || (thingKind == js::gc::FINALIZE_SHORT_STRING)", file=0x101d2cf18 "/Users/idefix2/moz/ws5/js/src/jsgcinlines.h", ln=66) at /Users/idefix2/moz/ws5/js/src/jsutil.cpp:80
80	    *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  0x00000001018fb4fc in JS_Assert (s=0x101d2cf70 "(thingKind == js::gc::FINALIZE_STRING) || (thingKind == js::gc::FINALIZE_SHORT_STRING)", file=0x101d2cf18 "/Users/idefix2/moz/ws5/js/src/jsgcinlines.h", ln=66) at /Users/idefix2/moz/ws5/js/src/jsutil.cpp:80
#1  0x0000000101867f1f in NewFinalizableGCThing<JSFunction> (cx=0x122cd7ca0, thingKind=1) at jsgcinlines.h:64
#2  0x0000000101868014 in js_NewGCFunction (cx=0x122cd7ca0) at jsgcinlines.h:115
#3  0x00000001017f1e40 in js::detail::NewObject<false, true> (cx=0x122cd7ca0, clasp=0x1028a9920, proto=0x11be8a390, parent=0x11beaa2a0) at jsobjinlines.h:959
#4  0x00000001017f1f2f in js::NewFunction (cx=0x122cd7ca0, parent=0x11beaa2a0) at jsobjinlines.h:990
#5  0x00000001017f1f9b in js_NewFunction (cx=0x122cd7ca0, funobj=0x0, native=0x1009deee2 <nsDOMWorkerFunctions::Dump(JSContext*, unsigned int, jsval_layout*)>, nargs=1, flags=0, parent=0x11beaa2a0, atom=0x117f05b80) at /Users/idefix2/moz/ws5/js/src/jsfun.cpp:2869
#6  0x00000001017f21d5 in js_DefineFunction (cx=0x122cd7ca0, obj=0x11beaa2a0, atom=0x117f05b80, native=0x1009deee2 <nsDOMWorkerFunctions::Dump(JSContext*, unsigned int, jsval_layout*)>, nargs=1, attrs=0) at /Users/idefix2/moz/ws5/js/src/jsfun.cpp:3023
#7  0x0000000101786a3e in JS_DefineFunction (cx=0x122cd7ca0, obj=0x11beaa2a0, name=0x101ca3980 "dump", call=0x1009deee2 <nsDOMWorkerFunctions::Dump(JSContext*, unsigned int, jsval_layout*)>, nargs=1, attrs=0) at /Users/idefix2/moz/ws5/js/src/jsapi.cpp:4273
#8  0x0000000101786c23 in JS_DefineFunctions (cx=0x122cd7ca0, obj=0x11beaa2a0, fs=0x102894a80) at /Users/idefix2/moz/ws5/js/src/jsapi.cpp:4259
#9  0x00000001009e233b in nsDOMWorker::CompileGlobalObject (this=0x122b1e1e0, aCx=0x122cd7ca0) at /Users/idefix2/moz/ws5/dom/src/threads/nsDOMWorker.cpp:1636
#10 0x00000001009e2857 in nsDOMWorker::SetGlobalForContext (this=0x122b1e1e0, aCx=0x122cd7ca0) at /Users/idefix2/moz/ws5/dom/src/threads/nsDOMWorker.cpp:1567
#11 0x00000001009d9330 in nsDOMWorkerRunnable::Run (this=0x123ab3750) at /Users/idefix2/moz/ws5/dom/src/threads/nsDOMThreadService.cpp:400
#12 0x00000001015b7675 in nsThreadPool::Run (this=0x122b1eb90) at /Users/idefix2/moz/ws5/xpcom/threads/nsThreadPool.cpp:221
#13 0x00000001015b36de in nsThread::ProcessNextEvent (this=0x123ab37e0, mayWait=1, result=0x121a80e3c) at /Users/idefix2/moz/ws5/xpcom/threads/nsThread.cpp:547
#14 0x000000010153c468 in NS_ProcessNextEvent_P (thread=0x123ab37e0, mayWait=1) at nsThreadUtils.cpp:250
#15 0x00000001015b4083 in nsThread::ThreadFunc (arg=0x123ab37e0) at /Users/idefix2/moz/ws5/xpcom/threads/nsThread.cpp:263
#16 0x000000010509ee49 in _pt_root (arg=0x123aab740) at /Users/idefix2/moz/ws5/nsprpub/pr/src/pthreads/ptthread.c:228
#17 0x00007fff802da456 in _pthread_start ()
#18 0x00007fff802da309 in thread_start ()
(Assignee)

Comment 1

8 years ago
Gregor, what test reproduces this?
Assignee: general → jorendorff
(Assignee)

Comment 2

8 years ago
Created attachment 472769 [details] [diff] [review]
WIP 1 - untested

Gregor, I haven't tested this at all; I can only vouch that dom/src/threads compiles with it.

I have to run an errand right now but I'll come back to this later today.
(Reporter)

Comment 3

8 years ago
Created attachment 472777 [details]
stack

This leads now to another assertion. Stack in the attachment.
(Assignee)

Comment 4

8 years ago
Figures. OK, I will fix this tonight.

(Gregor mentioned on IRC that the failing test is in dom/thread/tests; presumably all of them would fail with this assertion.)
(Assignee)

Comment 5

8 years ago
Created attachment 472903 [details] [diff] [review]
WIP 2 - also untested

This is as far as I got tonight, unfortunately. Family at point of mutiny. More tomorrow.
Attachment #472769 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
WIP 2 doesn't cover the case where mGlobal is already set. I'm testing a fix for that now...
(Reporter)

Updated

8 years ago
Blocks: 594455
(Assignee)

Comment 7

8 years ago
Created attachment 473137 [details] [diff] [review]
v3
Attachment #472903 - Attachment is obsolete: true
Attachment #473137 - Flags: review?(bent.mozilla)
Comment on attachment 473137 [details] [diff] [review]
v3

>+      nsLazyAutoRequest ar;
>+      JSAutoCrossCompartmentCall axcc;
> 
>+      // Tell the worker which context it will be using
>+      if (mWorker->SetGlobalForContext(cx, &ar, &axcc)) {

Can you somehow assert here that the JSAutoCrossCompartmentCall and nsLazyAutoRequest are in the right state? Just so we don't break in the future.

>+      else {

And here?

>+  // belong to the caller. But on failure, we must not remain in a request or
>+  // cross-compartment call. So we enter both only locally at first. On

This is the only part I don't understand... Why must we not be in a request/ccc if we fail? I'll take your word for it though.
Attachment #473137 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 9

8 years ago
(In reply to comment #8)
> This is the only part I don't understand... Why must we not be in a request/ccc
> if we fail? I'll take your word for it though.

In short: lofty software-engineering ideals.

It seems only sane for a method to have a contract that leaves the caller in some well-defined state on failure. What we don't want is to have some failure paths that leave you in a ccc and some failure paths that don't. That kind of weirdness can cause tricky bugs. And if your test suite doesn't hit every failure path, you might not even notice right away.

What state should this method leave you in after failure? Again, this is pretty abstract, but it's sanest for methods to either succeed with a state change or fail with no state change (like a transaction).
(Assignee)

Comment 11

8 years ago
http://hg.mozilla.org/tracemonkey/rev/3a1ca1e40c07

I added this:

diff --git a/dom/src/threads/nsDOMThreadService.cpp b/dom/src/threads/nsDOMThreadService.cpp
--- a/dom/src/threads/nsDOMThreadService.cpp
+++ b/dom/src/threads/nsDOMThreadService.cpp
@@ -396,24 +396,30 @@ public:

     PRBool killWorkerWhenDone;
     {
       nsLazyAutoRequest ar;
       JSAutoCrossCompartmentCall axcc;

       // Tell the worker which context it will be using
       if (mWorker->SetGlobalForContext(cx, &ar, &axcc)) {
+        NS_ASSERTION(ar.entered(), "SetGlobalForContext must enter request on success");
+        NS_ASSERTION(axcc.entered(), "SetGlobalForContext must enter xcc on success");
+
         RunQueue(cx, &killWorkerWhenDone);

         // Remove the global object from the context so that it might be garbage
         // collected.
         JS_SetGlobalObject(cx, NULL);
         JS_SetContextPrivate(cx, NULL);
       }
       else {
+        NS_ASSERTION(!ar.entered(), "SetGlobalForContext must not enter request on failure");
+        NS_ASSERTION(!axcc.entered(), "SetGlobalForContext must not enter xcc on failure");
+
         {
           // Code in XPConnect assumes that the context's global object won't be
           // replaced outside of a request.
           JSAutoRequest ar2(cx);

           // This is usually due to a parse error in the worker script...
           JS_SetGlobalObject(cx, NULL);
           JS_SetContextPrivate(cx, NULL);
diff --git a/dom/src/threads/nsDOMWorker.h b/dom/src/threads/nsDOMWorker.h
--- a/dom/src/threads/nsDOMWorker.h
+++ b/dom/src/threads/nsDOMWorker.h
@@ -115,16 +115,18 @@ public:
       JS_EndRequest(mCx);
   }

   void enter(JSContext *aCx) {
     JS_BeginRequest(aCx);
     mCx = aCx;
   }

+  bool entered() const { return mCx != nsnull; }
+
   void swap(nsLazyAutoRequest &other) {
     JSContext *tmp = mCx;
     mCx = other.mCx;
     other.mCx = tmp;
   }

 private:
   JSContext *mCx;
diff --git a/js/src/jsapi.h b/js/src/jsapi.h
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -963,16 +963,18 @@ JS_END_EXTERN_C
 class JS_PUBLIC_API(JSAutoCrossCompartmentCall)
 {
     JSCrossCompartmentCall *call;
   public:
     JSAutoCrossCompartmentCall() : call(NULL) {}

     bool enter(JSContext *cx, JSObject *target);

+    bool entered() const { return call != NULL; }
+
     ~JSAutoCrossCompartmentCall() {
         if (call)
             JS_LeaveCrossCompartmentCall(call);
     }

     void swap(JSAutoCrossCompartmentCall &other) {
         JSCrossCompartmentCall *tmp = call;
         call = other.call;
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/3a1ca1e40c07
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.