Closed Bug 593742 Opened 10 years ago Closed 9 years ago

TM: nsDOMWorker has to set the right compartment

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: gwagner, Assigned: jorendorff)

References

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(2 files, 2 obsolete files)

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 ()
Gregor, what test reproduces this?
Assignee: general → jorendorff
Attached patch WIP 1 - untested (obsolete) — Splinter Review
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.
Attached file stack
This leads now to another assertion. Stack in the attachment.
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.)
Attached patch WIP 2 - also untested (obsolete) — Splinter Review
This is as far as I got tonight, unfortunately. Family at point of mutiny. More tomorrow.
Attachment #472769 - Attachment is obsolete: true
WIP 2 doesn't cover the case where mGlobal is already set. I'm testing a fix for that now...
Blocks: 594455
Attached patch v3Splinter Review
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+
(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).
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
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.