925437 - Implement WorkerNavigator.onLine

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

We are soon going to need to do work inside workers depending on whether we're offline or not, and that obviously requires us to *know* about our offline status in the worker context.

Is this something that we get to have these days with just a webidl change?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Well, you'd add it to dom/webidl/WorkerNavigator.webidl but you still need to implement in dom/workers/Navigator.cpp.  Not sure webidl can help with that last bit.  ;)

Comment 2

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

May or may not be easy based on what main thread asserts are hit :). I can work on this.

Comment 3

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] from comment #2)
> May or may not be easy based on what main thread asserts are hit :). I can
> work on this.

Thanks, Nikhil!

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #1)
> Well, you'd add it to dom/webidl/WorkerNavigator.webidl but you still need
> to implement in dom/workers/Navigator.cpp.  Not sure webidl can help with
> that last bit.  ;)

What?  You mean the WebIDL codegen cannot generate code to implement new features? o_O

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sometimes it can (e.g. unforgeable, replaceable, etc), but sometimes... no.  ;)

Comment 5

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

Attachment: Implement navigator.onLine on Workers.

I don't know how to test the suspend part of this code.

Comment 6

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

Comment on attachment 817940 [details] [diff] [review]
Implement navigator.onLine on Workers.

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

This needs to be rebased over the changes in bug 928312 (which removes the worker specific event code).  That shouldn't be hard, but I want to avoid bitrotting that as much as possible (it's a 300 KB patch).

You should add ononline/onoffline handlers to the Worker global.  This will be trivial after bug 928312.

Workers don't need this freeze/thawing stuff.  When a worker is suspended its event queue is blocked.  You can just shove an event in the queue and if the worker ever resumes it'll run appropriately.  The main thread needs this because we can't block the event queue.

::: dom/workers/RuntimeService.cpp
@@ +2198,5 @@
> +bool
> +RuntimeService::IsOffline()
> +{
> +  return mIsOffline;
> +}

So this is technically a run-to-completion semantics violation.  Because this is updated from the main thread, it's possible for a script that just polls navigator.onLine to see this change without yielding.

The correct way to do this would be for each worker to have its own copy of the offline status and to propagate the offline status downward to each worker via OfflineStatusChangedRunnable.

Comment 7

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

Going to wait until 928312 lands.

Comment 8

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

Attachment: Implement navigator.onLine on Workers. s

Comment 9

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

Comment on attachment 829624 [details] [diff] [review]
Implement navigator.onLine on Workers.  s

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

r-, mostly for the event ordering issue.  This looks pretty good.

::: dom/workers/Navigator.h
@@ +82,5 @@
> +
> +  void SetOnLine(bool aOnline)
> +  {
> +    mOnline = aOnline;
> +  }

This needs a threadsafety assertion.

::: dom/workers/WorkerPrivate.cpp
@@ +2935,5 @@
> +  globalScope->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
> +
> +  for (uint32_t index = 0; index < mChildWorkers.Length(); ++index) {
> +    mChildWorkers[index]->OfflineStatusChangeEvent(aCx, aStatus);
> +  }

I think this ordering is wrong.  Because we are broadcasting to our children after running our event handlers, any messages posted from the parent to the children from these event handlers will not see the updated state in the child.

::: dom/workers/WorkerPrivate.h
@@ +491,5 @@
>    void
>    GarbageCollect(JSContext* aCx, bool aShrinking);
>  
> +  void
> +  OfflineStatusChangeEvent(JSContext* aCx, const nsAString& aStatus);

Why are you using a string instead of a boolean?

Comment 10

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

Attachment: Implement navigator.onLine on Workers.

Reorder event dispatch - children first, then parent.
Use bool, I was using a string because it was easier to use that to get the event name in some earlier code, but now it does make sense to use bool.

Comment 11

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

Comment on attachment 830922 [details] [diff] [review]
Implement navigator.onLine on Workers.

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

r=me with the comments addressed.

Your test should test subworkers too (not just top level workers).

::: dom/workers/Navigator.h
@@ +83,5 @@
> +  }
> +
> +  void SetOnLine(bool aOnline)
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());

Well I really wanted to assert that we are on the right worker thread, but I guess we don't have a reference to the WorkerPrivate from Navigator.  I think you can drop this assertion (and the MainThreadUtils.h include).

::: dom/workers/WorkerPrivate.cpp
@@ +2914,5 @@
> +    eventType.AssignLiteral("online");
> +  }
> +
> +  nsRefPtr<WorkerNavigator> navigator = globalScope->Navigator();
> +  navigator->SetOnLine(!aIsOffline);

Calling WorkerScope::Navigator() will create the navigator if it does not already exist, which seems wasteful.  I think you should add a new method that does not create the navigator and use that here.  GetNavigator maybe, or GetExistingNavigator.

@@ +2928,5 @@
> +  event->SetTrusted(true);
> +
> +  globalScope->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
> +
> +}

Nit: extra new line at end of function.

Comment 12

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

While this works on all other platforms, on B2G the tests consistently ran into a weird importScripts() error.

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

Comment 13

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

Looking into this more, I inlined the _head.js script on child workers, and surprisingly, the workers never receive online/offline events. The HTML page running the workers does get the events. This is on B2G Emulator. Bug 840612 seems relevant and I'm going to build the emulator myself. Blake, any ideas would be great. Is the RuntimeService supposed to listen to some other observer events on B2G?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I spoke with Nikhil on IRC and it appears that we're sending the right observer notifications. He'll continue debugging to figure out where things fall apart.

Comment 15

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

Seems like the inlining was enough.

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

Comment 16

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

Attachment: Implement navigator.onLine on Workers.

Found a race condition.
If the worker script has never accessed `navigator`, WorkerNavigator is not created.
When offline status changes, the navigator.onLine state isn't changed.
If the offline/online event handler accesses `navigator`, it used to read the state from RuntimeService, which was changed on the main thread. So if the offline status changes twice quickly, it is possible for the navigator.onLine to go out of sync with the event received.

Modified to have WorkerScope hold the status and if there is a existing Navigator, pass the status to it, otherwise use it when the Navigator is created.

Comment 17

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

Attachment: Implement navigator.onLine on Workers.

Keeping a record in WorkerScope didn't allow a nice way to set the value on construction.
Moved to WorkerPrivate where it fits better with the preferences system.

Comment 18

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

Comment on attachment 8343832 [details] [diff] [review]
Implement navigator.onLine on Workers.

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

::: dom/workers/Navigator.h
@@ +9,5 @@
>  #include "nsString.h"
>  #include "nsWrapperCache.h"
>  
> +#include "Workers.h"
> +

Please don't rearrange this.

::: dom/workers/WorkerScope.h
@@ +65,5 @@
>    already_AddRefed<WorkerNavigator>
>    Navigator();
> +
> +  already_AddRefed<WorkerNavigator>
> +  GetExistingNavigator();

This should be const.

Comment 19

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df0f58eb2b6f

Comment 20

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

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

Comment 21

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorOnLine
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorOnLine.onLine

and

https://developer.mozilla.org/en-US/Firefox/Releases/29