998474 - "Assertion failure: current (Wrong thread!)" calling port.start() on main thread

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: current (Wrong thread!), at dom/workers/WorkerPrivate.cpp:5868

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrew McCreight [:mccr8]]

The main thread is running a timeout handler, which is triggering MessagePort::Start, which calls WorkerRunnable::Dispatch and triggers the assert.  The test case uses a SharedWorker.  Could you look into this, Andrea?

Comment 3

[Andrea Marchesini [:baku]]

Sure.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: crash.patch

This patch is still not enough because when WorkerRun() is called in WorkerRunnable::Run() we have this code:

  JS::Rooted<JSObject*> targetCompartmentObject(cx);
  if (targetIsWorkerThread) {
    targetCompartmentObject = JS::CurrentGlobalOrNull(cx);
  } else {
    targetCompartmentObject = mWorkerPrivate->GetWrapper();
  } 

but mWorkerPrivate->GetWrapper() returns null, so we don't enter into the right compartment.

Comment 5

[Andrew McCreight [:mccr8]]

Kyle, can you provide some feedback on this patch given that Ben is on PTO?  Thanks.  It has sat here for a month.

Comment 6

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

comment 4 says this patch is wrong, no?  What do you want me to do here?

Comment 7

[Andrew McCreight [:mccr8]]

Andrea, what sort of feedback are you looking for here to move this bug forward?

Comment 8

[Andrea Marchesini [:baku]]

Attachment: crash.patch

This patch works fine and if it's fully green on try I'm happier:

https://tbpl.mozilla.org/?tree=Try&rev=a45158613c81

Comment 9

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

Comment on attachment 8427625 [details] [diff] [review]
crash.patch

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

I don't understand this.  The Pre/PostDispatch bits are just copied from the base class.  Why is that?

Is the only change this patch makes that does something the AutoNoJSAPI bit?

Comment 10

[Andrea Marchesini [:baku]]

Attachment: crash.patch

This is more generic.

Comment 11

[Andrea Marchesini [:baku]]

> I don't understand this.  The Pre/PostDispatch bits are just copied from the
> base class.  Why is that?

In SharedWorkers preDispatch/PostDispatch run on the main-thread. This is the reason why I removed the check: AssertIsOnWorkerThread()

Comment 12

[Andrew McCreight [:mccr8]]

Kyle, can you review this or find somebody else to review it?  It has been sitting here for a month.

Comment 13

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

Comment on attachment 8427625 [details] [diff] [review]
crash.patch

r=me

Comment 14

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

Comment on attachment 8431419 [details] [diff] [review]
crash.patch

We want the earlier, narrower fix.

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/941b7b706401

Comment 16

[Andrew McCreight [:mccr8]]

If this affects more than trunk, it should have gotten sec-approval before landing.  What versions does this affect?  29 and later?

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/941b7b706401

Comment 18

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

Btw, this is not a race, it's just a bogus assertion.