1042908 - Worker error handling should trigger errors on main thread even if worker isn't accepting events.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Due to the IsAcceptingEvents() check before reporting errors to the ServiceWorkerManager or to connected SharedWorkers, sometimes those main thread objects never receive the error.

I am unassigning myself since I'll be on PTO. Ben if this looks good, would you go ahead and land it?

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix WorkerPrivate error handling

Comment 2

[Ryan VanderMeulen [:RyanVM]]

IIRC, this had Try failures when bz ran it through there last week (mochitest-4 IIRC). Please verify that this is green first.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix WorkerPrivate error handling

Gate acquiring inner window id on worker accepting events.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 8

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

Comment on attachment 8487439 [details] [diff] [review]
Fix WorkerPrivate error handling

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

::: dom/workers/WorkerPrivate.cpp
@@ +1315,5 @@
>  
> +      // The innerWindowId is only required if we are going to ReportError
> +      // below, which is gated on this condition. If inner window correctness
> +      // check is only going to succeed when the worker is accepting events.
> +      if (aWorkerPrivate->IsAcceptingEvents()) {

Rather than call this twice (it grabs a lock) just call it once and put a stack var at the top with the result.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e03c9e5816bb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e03c9e5816bb