793020 - ###!!! ASSERTION: DOMBindingBase not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file ../../dist/include/mozilla/dom/workers/bindings/DOMBindingBase.h, line 49

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

>	xul.dll!NS_DebugBreak_P(unsigned int aSeverity=1, const char * aStr=0x546ad1c8, const char * aExpr=0x53e60b94, const char * aFile=0x546c37d0, int aLine=49)  Line 285	C++
 	xul.dll!mozilla::dom::workers::DOMBindingBase::AddRef()  Line 49 + 0x8a bytes	C++
 	xul.dll!nsRefPtr<mozilla::dom::workers::WorkerPrivate>::nsRefPtr<mozilla::dom::workers::WorkerPrivate>(mozilla::dom::workers::WorkerPrivate * aRawPtr=0x16009178)  Line 899	C++
 	xul.dll!`anonymous namespace'::LogViolationDetailsRunnable::LogViolationDetailsRunnable(mozilla::dom::workers::WorkerPrivate * aWorker=0x16009178, const nsString & aFileName={...}, unsigned int aLineNum=7)  Line 268 + 0x27 bytes	C++
 	xul.dll!`anonymous namespace'::ContentSecurityPolicyAllows(JSContext * aCx=0x13ac2b88)  Line 313 + 0x27 bytes	C++
 	mozjs.dll!js::GlobalObject::isRuntimeCodeGenEnabled(JSContext * cx=0x13ac2b88)  Line 551 + 0xd bytes	C++
 	mozjs.dll!EvalKernel(JSContext * cx=0x13ac2b88, const JS::CallArgs & args={...}, EvalType evalType=DIRECT_EVAL, js::StackFrame * caller=0x1a010040, JS::Handle<JSObject *> scopeobj={...})  Line 163 + 0x1a bytes	C++
 	mozjs.dll!js::DirectEval(JSContext * cx=0x13ac2b88, const JS::CallArgs & args={...})  Line 332 + 0x22 bytes	C++
 	mozjs.dll!js::Interpret(JSContext * cx=0x13ac2b88, js::StackFrame * entryFrame=0x1a010040, js::InterpMode interpMode=JSINTERP_NORMAL)  Line 2420 + 0x10 bytes	C++
 	mozjs.dll!js::RunScript(JSContext * cx=0x13ac2b88, JS::Handle<JSScript *> script={...}, js::StackFrame * fp=0x1a010040)  Line 324 + 0xf bytes	C++
 	mozjs.dll!js::InvokeKernel(JSContext * cx=0x13ac2b88, JS::CallArgs args={...}, js::MaybeConstruct construct=NO_CONSTRUCT)  Line 378 + 0x28 bytes	C++
 	mozjs.dll!js::Invoke(JSContext * cx=0x13ac2b88, js::InvokeArgsGuard & args={...}, js::MaybeConstruct construct=NO_CONSTRUCT)  Line 109 + 0x37 bytes	C++
 	mozjs.dll!js::Invoke(JSContext * cx=0x13ac2b88, const JS::Value & thisv={...}, const JS::Value & fval={...}, unsigned int argc=1, JS::Value * argv=0x18cdf968, JS::Value * rval=0x18cdf970)  Line 411 + 0xf bytes	C++
 	mozjs.dll!JS_CallFunctionValue(JSContext * cx=0x13ac2b88, JSObject * objArg=0x1c609040, JS::Value fval={...}, unsigned int argc=1, JS::Value * argv=0x18cdf968, JS::Value * rval=0x18cdf970)  Line 5852 + 0x34 bytes	C++
 	xul.dll!mozilla::dom::workers::EventListenerManager::DispatchEvent(JSContext * aCx=0x13ac2b88, const mozilla::dom::workers::EventTarget & aTarget={...}, JSObject * aEvent=0x1c609310, mozilla::ErrorResult & aRv={...})  Line 406 + 0x3a bytes	C++
 	xul.dll!mozilla::dom::workers::EventTarget::DispatchEvent(JSObject * aEvent=0x1c609310, mozilla::ErrorResult & aRv={...})  Line 53	C++
 	xul.dll!mozilla::dom::EventTargetBinding_workers::dispatchEvent(JSContext * cx=0x13ac2b88, JS::Handle<JSObject *> obj={...}, mozilla::dom::workers::EventTarget * self=0x13ab5eb0, unsigned int argc=1, JS::Value * vp=0x1a010010)  Line 552 + 0x10 bytes	C++
 	xul.dll!mozilla::dom::EventTargetBinding_workers::genericMethod(JSContext * cx=0x13ac2b88, unsigned int argc=1, JS::Value * vp=0x1a010010)  Line 586 + 0x21 bytes	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx=0x13ac2b88, int (JSContext *, unsigned int, JS::Value *)* native=0x538d6980, const JS::CallArgs & args={...})  Line 370 + 0x19 bytes	C++
 	mozjs.dll!js::InvokeKernel(JSContext * cx=0x13ac2b88, JS::CallArgs args={...}, js::MaybeConstruct construct=NO_CONSTRUCT)  Line 367 + 0x1d bytes	C++
 	mozjs.dll!js::Invoke(JSContext * cx=0x13ac2b88, js::InvokeArgsGuard & args={...}, js::MaybeConstruct construct=NO_CONSTRUCT)  Line 109 + 0x37 bytes	C++
 	mozjs.dll!js::Invoke(JSContext * cx=0x13ac2b88, const JS::Value & thisv={...}, const JS::Value & fval={...}, unsigned int argc=1, JS::Value * argv=0x18cdfc6c, JS::Value * rval=0x18cdfc74)  Line 411 + 0xf bytes	C++
 	mozjs.dll!JS_CallFunctionName(JSContext * cx=0x13ac2b88, JSObject * objArg=0x1c609040, const char * name=0x546af3bc, unsigned int argc=1, JS::Value * argv=0x18cdfc6c, JS::Value * rval=0x18cdfc74)  Line 5838 + 0x7c bytes	C++
 	xul.dll!mozilla::dom::workers::events::DispatchEventToTarget(JSContext * aCx=0x13ac2b88, JSObject * aTarget=0x1c609040, JSObject * aEvent=0x1c609310, bool * aPreventDefaultCalled=0x18cdfcba)  Line 1082 + 0x28 bytes	C++
 	xul.dll!`anonymous namespace'::MessageEventRunnable::WorkerRun(JSContext * aCx=0x13ac2b88, mozilla::dom::workers::WorkerPrivate * aWorkerPrivate=0x16009178)  Line 821 + 0x15 bytes	C++
 	xul.dll!mozilla::dom::workers::WorkerRunnable::Run()  Line 1798 + 0x18 bytes	C++
 	xul.dll!mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext * aCx=0x13ac2b88)  Line 2786	C++
 	xul.dll!`anonymous namespace'::WorkerThreadRunnable::Run()  Line 412	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=true, bool * result=0x18cdfe87)  Line 612 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x137eb3a8, bool mayWait=true)  Line 220 + 0x17 bytes	C++
 	xul.dll!nsThread::ThreadFunc(void * arg=0x137eb3a8)  Line 256 + 0xb bytes	C++
 	nspr4.dll!_PR_NativeRunThread(void * arg=0x145bfef0)  Line 395 + 0xf bytes	C
 	nspr4.dll!pr_root(void * arg=0x145bfef0)  Line 90 + 0xf bytes	C
 	msvcr100d.dll!__beginthreadex()  + 0x243 bytes	
 	msvcr100d.dll!__beginthreadex()  + 0x1d4 bytes

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

All you need to do is run test_csp.html to hit this.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 1

Is there a reason why DOMBindingBase was not Thread-Safe?
It seems there is not a 'clean' way to share WorkerPrivate between threads. is it?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch 2

This is another solution:
LogViolationDetailsRunnable runs synchronously, so we don't need to use nsRefPtr.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I like the second approach more, but I think bent should review it.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 663316 [details] [diff] [review]
patch 1

I don't think this is the right solution, but if it is, I bitrotted this in Bug 793025 :-P

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 663369 [details] [diff] [review]
patch 2

Going to let bent review this.  I'm not very familiar with sync queues.

Comment 7

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

(In reply to Andrea Marchesini (:baku) from comment #2)
> Is there a reason why DOMBindingBase was not Thread-Safe?
> It seems there is not a 'clean' way to share WorkerPrivate between threads.

The reason they do not get threadsafe refcounting it that these objects really can't be shared across threads without serious care.

Comment 8

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

Comment on attachment 663369 [details] [diff] [review]
patch 2

Review of attachment 663369 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/RuntimeService.cpp
@@ +258,4 @@
>    nsString mFileName;
>    uint32_t mLineNum;
>  
> +  uint32_t mSyncQueueKey;

Nit: No need for the extra line above.

@@ +269,5 @@
> +    LogViolationDetailsResponseRunnable(WorkerPrivate* aWorkerPrivate,
> +                                        uint32_t aSyncQueueKey)
> +    : WorkerSyncRunnable(aWorkerPrivate,
> +                         aSyncQueueKey,
> +                         false),

Nit: Squeeze as many args as you can onto one line.

@@ +369,5 @@
>    }
>  
> +  nsRefPtr<LogViolationDetailsRunnable> runnable =
> +      new LogViolationDetailsRunnable(worker, fileName, lineNum);
> +  runnable->Dispatch(aCx);

Need to do this:

  if (!runnable->Dispatch(aCx)) {
    JS_ReportPendingException(aCx);
  }

::: dom/workers/WorkerPrivate.cpp
@@ +2223,5 @@
>    SwapToISupportsArray(mScriptNotify, aDoomed);
>    SwapToISupportsArray(mBaseURI, aDoomed);
>    SwapToISupportsArray(mScriptURI, aDoomed);
>    SwapToISupportsArray(mPrincipal, aDoomed);
> +  SwapToISupportsArray(mCSP, aDoomed);

Please update the SetCapacity call above.

Comment 9

[Andrea Marchesini [:baku]]

> Please update the SetCapacity call above.

Currently we have: SetCapacity(7) and we store 7 items...

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 2b

Comment 11

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

(In reply to Andrea Marchesini (:baku) from comment #9)
> Currently we have: SetCapacity(7) and we store 7 items...

Oh, right. smaug removed one a while back. Sorry about that.

Comment 12

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

Comment on attachment 663526 [details] [diff] [review]
patch 2b

Review of attachment 663526 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks.

Comment 13

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=d6d68963e80e green on try.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/147fc5a585bd

Comment 15

[Andrea Marchesini [:baku]]

we have an existing mochitest: dom/workers/test/test_csp.html

Comment 16

[Andrew McCreight [:mccr8]]

Can we make these assertions fatal-er so they will burn the tree?

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We can, and my patch in Bug 753659 does that, but we have to fix them all first.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/147fc5a585bd

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This was trunk only, and it's been fixed for almost two weeks.  Opening.