Closed Bug 998474 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Workers, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox33 --- fixed

People

(Reporter: jruderman, Assigned: baku)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 2 obsolete files)

Attached file testcase
Assertion failure: current (Wrong thread!), at dom/workers/WorkerPrivate.cpp:5868
Attached file stack
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?
Flags: needinfo?(amarchesini)
Sure.
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Attached patch crash.patch (obsolete) — Splinter Review
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.
Attachment #8411157 - Flags: feedback?(bent.mozilla)
Group: dom-core-security
Kyle, can you provide some feedback on this patch given that Ben is on PTO?  Thanks.  It has sat here for a month.
Flags: needinfo?(khuey)
comment 4 says this patch is wrong, no?  What do you want me to do here?
Flags: needinfo?(khuey)
Andrea, what sort of feedback are you looking for here to move this bug forward?
Flags: needinfo?(amarchesini)
Attached patch crash.patchSplinter Review
This patch works fine and if it's fully green on try I'm happier:

https://tbpl.mozilla.org/?tree=Try&rev=a45158613c81
Attachment #8411157 - Attachment is obsolete: true
Attachment #8411157 - Flags: feedback?(bent.mozilla)
Attachment #8427625 - Flags: review?(khuey)
Flags: needinfo?(amarchesini)
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?
Attached patch crash.patch (obsolete) — Splinter Review
This is more generic.
Attachment #8427625 - Attachment is obsolete: true
Attachment #8427625 - Flags: review?(khuey)
Attachment #8431419 - Flags: review?(khuey)
Flags: needinfo?(amarchesini)
> 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()
Kyle, can you review this or find somebody else to review it?  It has been sitting here for a month.
Flags: needinfo?(khuey)
Comment on attachment 8427625 [details] [diff] [review]
crash.patch

r=me
Attachment #8427625 - Attachment is obsolete: false
Attachment #8427625 - Flags: review+
Flags: needinfo?(khuey)
Comment on attachment 8431419 [details] [diff] [review]
crash.patch

We want the earlier, narrower fix.
Attachment #8431419 - Attachment is obsolete: true
Attachment #8431419 - Flags: review?(khuey)
If this affects more than trunk, it should have gotten sec-approval before landing.  What versions does this affect?  29 and later?
https://hg.mozilla.org/mozilla-central/rev/941b7b706401
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Group: dom-core-security
Btw, this is not a race, it's just a bogus assertion.
Flags: needinfo?(continuation)
Group: core-security
Flags: needinfo?(continuation)
You need to log in before you can comment on or make changes to this bug.