Gregor Wagner [:gwagner] Reporter
	Description • 14 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 ()

	Jason Orendorff [:jorendorff] Assignee
	Comment 1 • 14 years ago

Gregor, what test reproduces this?

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 14 years ago

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.

	Gregor Wagner [:gwagner] Reporter
	Comment 3 • 14 years ago

This leads now to another assertion. Stack in the attachment.

	Jason Orendorff [:jorendorff] Assignee
	Comment 4 • 14 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.)

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 14 years ago

This is as far as I got tonight, unfortunately. Family at point of mutiny. More tomorrow.

	Jason Orendorff [:jorendorff] Assignee
	Comment 6 • 14 years ago

WIP 2 doesn't cover the case where mGlobal is already set. I'm testing a fix for that now...

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 8 • 14 years ago

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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 9 • 14 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).

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 10 • 14 years ago

Sounds fine to me!

	Jason Orendorff [:jorendorff] Assignee
	Comment 11 • 14 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;

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 12 • 14 years ago

(In reply to comment #11)

Thanks!

	Chris Leary [:cdleary] (not checking bugmail)
	Comment 13 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/3a1ca1e40c07