1323172 - navigator.connection not exposed in Workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P3)

Description

[stefhak]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/602.2.14 (KHTML, like Gecko) Version/10.0.1 Safari/602.2.14

Steps to reproduce:

Just test for navigator.connection in a Service Worker script.

The spec says it should be exposed to Workers (which includes the SW).


Actual results:

not found


Expected results:

should be there

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This affects all worker types and not just service workers.

Comment 2

[Andrew Overholt [:overholt]]

baku may be interested in taking this.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - workers

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Before I review this, should we determine if we want to conform to the other parts of the spec here?  For example, we have `ontypechange` while the spec has `onchange`, etc.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - spec

Here a second patch for the spec diff.
We don't have downlinkMax info (nor connectionType actually).

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - spec

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8818578 [details] [diff] [review]
part 2 - spec

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

So I think we should do this, but I wonder if others have objections.  It seems like the sort of thing that could have been contentious in the firefoxos days.  Can you write an intent-to-implement or intent-to-ship?

Also, I think zero for the downlinkMax is probably not the right value.  The spec gives us numbers we can use without perfect knowledge of the environment.

::: dom/network/Connection.h
@@ +52,5 @@
>  
>    ConnectionType Type() const { return mType; }
>  
> +  // We don't actually know the downlink/connectionType via HAL.
> +  double DownlinkMax() const { return 0; }

I don't think we can return 0 here.  The spec has a table we can use to approximate the downlinkMax based on the type:

http://wicg.github.io/netinfo/#dfn-underlying-connection-technology

So if we take the most liberal numbers:

* wimax is 365mb
* wifi is 7000mb
* cellular is 100mb
* bluetooth is 24mb
* etherenet is 10000mb
* unknown is +infinity
* none is zero

We could write an android bug to get better granularity here by reporting the type of cellular or wifi the device has.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8818569 [details] [diff] [review]
part 1 - workers

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

::: dom/network/Connection.cpp
@@ +48,5 @@
>  
>  void
>  Connection::Shutdown()
>  {
> +  if (mBeenShutDown) {

NS_ASSERT_OWNINGTHREAD(Connection)

::: dom/network/ConnectionWorker.cpp
@@ +13,5 @@
> +namespace dom {
> +namespace network {
> +
> +class ConnectionProxy final : public NetworkObserver
> +                            , public WorkerHolder

Is the WorkerHolder required for the sync runnables?  It seems otherwise we don't really need to hold the worker alive here.

@@ +168,5 @@
> +{
> +  RefPtr<ConnectionWorker> c = new ConnectionWorker(aWorkerPrivate);
> +  c->mProxy = ConnectionProxy::Create(aWorkerPrivate, c);
> +  if (!c->mProxy) {
> +    aRv.Throw(NS_ERROR_FAILURE);

Can you make this a TypeError with a message like "The Worker thread is shutting down."  We can probably use that message in a bunch of places.

@@ +198,5 @@
> +void
> +ConnectionWorker::ShutdownInternal()
> +{
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  mProxy->Shutdown();

So this is only called in the case where the WorkerNavigator is destroyed, right?  And typically the proxy should already be destroyed since its WorkerHolder would stop the shutdown of the worker thread.

Can we assert here that the proxy is shutdown?

::: dom/network/ConnectionWorker.h
@@ +18,5 @@
> +class ConnectionProxy;
> +
> +class ConnectionWorker final : public Connection
> +{
> +  friend class ConnectionProxy;

If you just put ConnectionProxy in ConnectionWorker then you would not need a friend here.

class ConnectionWorker
{
  class Proxy;
};

class ConnectionWorker::Proxy
{
};

::: dom/network/tests/mochitest.ini
@@ +5,5 @@
>  
>  [test_network_basics.html]
>  skip-if = toolkit == 'android'
> +[test_network_basics_worker.html]
> +skip-if = toolkit == 'android'

Why are these skipped on android?  AFAIK thats the only place we actually use this API.

::: dom/webidl/WorkerNavigator.webidl
@@ +14,5 @@
>  WorkerNavigator implements NavigatorStorage;
> +
> +// http://wicg.github.io/netinfo/#extensions-to-the-navigator-interface
> +[Exposed=(Worker)]
> +partial interface WorkerNavigator {

Can you write a spec issue to move this to WindowOrWorker shared interface?

::: dom/workers/WorkerNavigator.cpp
@@ +31,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CLASS(WorkerNavigator)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(WorkerNavigator)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mStorageManager)
> +  tmp->mConnection->Shutdown();

Why is this necessary?  The ~Connection() destructor should call this, right?

@@ +52,5 @@
> +                                 bool aOnline)
> +  : mProperties(aProperties)
> +  , mOnline(aOnline)
> +{
> +  MOZ_COUNT_CTOR(WorkerNavigator);

I don't think MOZ_COUNT_(CTOR|DTOR) should be used if you are also using our refcounting macros.  They do their own leak detection via NS_LOG_ADDREF.  So I guess just drop this.

Comment 9

[Andrea Marchesini [:baku]]

If you don't mind I prefer to move the second patch in a separate bug.
I want to land patch 1 immediately because, at least, we are consistent (consistently wrong, but consistent) in main-thread and workers.

Comment 10

[Andrea Marchesini [:baku]]

> > +class ConnectionProxy final : public NetworkObserver
> > +                            , public WorkerHolder
> 
> Is the WorkerHolder required for the sync runnables?  It seems otherwise we
> don't really need to hold the worker alive here.

It's for the notification. When, on the main-thread, a notification event is received, we dispatch a runnable to the worker thread to propagate the information. I need to keep the worker alive.

> If you just put ConnectionProxy in ConnectionWorker then you would not need
> a friend here.

Right, but I have to make it public because otherwise all the runnables cannot use it.
I don't like it so much...

> Why are these skipped on android?  AFAIK thats the only place we actually
> use this API.

heh. I just followed the pattern. I guess it's because the 'type' value can change?
I'll see if the test works correctly on android and I'll enable it in case everything is fine.
Follow up.


> Can you write a spec issue to move this to WindowOrWorker shared interface?

We don't have such interface. We have WindowOrWorkerGlobalScope. Probably it's better to do:

[NoInterfaceObject, Exposed=(Window,Worker)]
interface NavigatorNetworkInformation {
  readonly attribute NetworkInformation connection;
};

Navigator implements NavigatorNetworkInformation;
WorkerNavigator implements NavigatorNetworkInformation;

https://github.com/WICG/netinfo/issues/45

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f305096ee45
Expose NetworkInformation interface to workers, r=bkelly

Comment 12

[Carsten Book [:Tomcat]]

sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=40629921&repo=mozilla-inbound

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5d50943bbc
Backed out changeset 8f305096ee45 for crashes in /test_tcpsocket_enabled_no_perm.html

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7638db235fd
Expose NetworkInformation interface to workers, r=bkelly

Comment 15

[Carsten Book [:Tomcat]]

backed out again for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=40637701&repo=mozilla-inbound

Baku, since this bounced now twice can you run a try run before the next try to this landed, thanks!

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5b3408af8f
Backed out changeset b7638db235fd for test bustage in dom/workers/test/test_navigator.html on a CLOSED TREE

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #9)
> If you don't mind I prefer to move the second patch in a separate bug.
> I want to land patch 1 immediately because, at least, we are consistent
> (consistently wrong, but consistent) in main-thread and workers.

Sure.

(In reply to Andrea Marchesini [:baku] from comment #10)
> > > +class ConnectionProxy final : public NetworkObserver
> > > +                            , public WorkerHolder
> > 
> > Is the WorkerHolder required for the sync runnables?  It seems otherwise we
> > don't really need to hold the worker alive here.
> 
> It's for the notification. When, on the main-thread, a notification event is
> received, we dispatch a runnable to the worker thread to propagate the
> information. I need to keep the worker alive.

Well, the Dispatch() would just fail, right?  I don't think we crash in that case, do we?  Failing the dispatch seems fine to me.

Comment 18

[Andrea Marchesini [:baku]]

> Well, the Dispatch() would just fail, right?  I don't think we crash in that
> case, do we?  Failing the dispatch seems fine to me.

Actually, on the shutdown of a worker, the sync runnables all fails if there is not a feature to keep it alive.
And if we don't remove the network observer, at the next notification we have an invalid WorkerPrivate pointer and we would end up doing funny things :)

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #18)
> > Well, the Dispatch() would just fail, right?  I don't think we crash in that
> > case, do we?  Failing the dispatch seems fine to me.
> 
> Actually, on the shutdown of a worker, the sync runnables all fails if there
> is not a feature to keep it alive.
> And if we don't remove the network observer, at the next notification we
> have an invalid WorkerPrivate pointer and we would end up doing funny things
> :)

Fair enough.  Thanks.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaeb9bcf954a
Expose NetworkInformation interface to workers, r=bkelly

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75cb10a1f69f
Fix some mochitest failures, r=me

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aaeb9bcf954a
https://hg.mozilla.org/mozilla-central/rev/75cb10a1f69f

Comment 23

[(no longer active)]

Andrea, can you please hold off on these changes before the discussion in dev-platform is finished?  Based on the current state of the discussion, it seems unlikely that we want to be exposing this API to more contexts at this point.  :(

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #23)
> Andrea, can you please hold off on these changes before the discussion in
> dev-platform is finished?  Based on the current state of the discussion, it
> seems unlikely that we want to be exposing this API to more contexts at this
> point.  :(

If its exposed on Window there is no reason to exclude it from Workers.  If you want to remove it from Window as well, then I have no objection to removing it from Workers.

Comment 25

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #24)
> (In reply to :Ehsan Akhgari from comment #23)
> > Andrea, can you please hold off on these changes before the discussion in
> > dev-platform is finished?  Based on the current state of the discussion, it
> > seems unlikely that we want to be exposing this API to more contexts at this
> > point.  :(
> 
> If its exposed on Window there is no reason to exclude it from Workers.  If
> you want to remove it from Window as well, then I have no objection to
> removing it from Workers.

The reason to exclude it from workers is to have one fewer place to eliminate the usage in the future if we decide to unship eventually, or ship an incompatible API with the current spec.  Why the rush to expose it to workers when the whole API exposure is under active discussion?

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

> The reason to exclude it from workers is to have one fewer place to
> eliminate the usage in the future if we decide to unship eventually, or ship
> an incompatible API with the current spec.  Why the rush to expose it to
> workers when the whole API exposure is under active discussion?

So lets get this straight:

1. Developer asks for thing we already ship to be exposed on workers so its more compatible with other browsers
2. We discuss
3. Instead we remove the feature completely

Have we received even a single complaint from users or developers about this feature?  Is there any measurable problem being solved by removing this?  Or is it totally internal theorizing and bikeshedding?

I find this entire situation bizarre and frustrating.

Comment 27

[(no longer active)]

As far as I can tell, we're still discussing.  Also please don't blame me for this situation, I merely provided historical context on the thread.  I am not advocating for anything specific, just for not changing things as the discussion is happening to avoid further confusion...  As far as I'm concerned the ship on having exposed the API sailed back when Fennec implemented the old version of the API years ago.

Comment 28

[Andrea Marchesini [:baku]]

The update of the interface is on hold. This patch was about exposing the current API to workers. It's disabled by pref everywhere except android. I think we don't have to backout this patch. We will take a decision about having or not this API in time for the next aurora :)

Comment 29

[stefhak]

Thanks a lot all of you who helped getting this implemented. I just tested and it works just fine for my use case. IMO there is no worry that fields like downlinkMax are not reported - what others do is simply report them with the value in the table of the spec for the current "type" - and then you can just as well go with only the "type" field.

Comment 30

[stefhak]

BTW, how do I enable if I'd like to test on a desktop version of Nightly?

Comment 31

[Chris Mills [:cmills]]

(In reply to stefhak from comment #30)
> BTW, how do I enable if I'd like to test on a desktop version of Nightly?

I don't think it is available in Nightly at all, although I could be wrong. I tested using WebIDE and remote debugging (https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_over_Wifi)

Comment 32

[Chris Mills [:cmills]]

Docs work on this:

I've updated existing related pages to mention the features are available in workers, as appropriate:

https://developer.mozilla.org/en-US/docs/Web/API/Network_Information_API
https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation
https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/type
https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/onchange
https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/downlinkMax

https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Functions_and_classes_available_to_workers

I've created a new WorkerNavigator.connection page (I can easily move this if we have to make changes later due to current discussions):

https://developer.mozilla.org/en-US/docs/Web/API/WorkerNavigator/connection

I've also added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM

A couple of notes:

* downlinkMax does seem to work on FxA yet, although it does in Chrome Android
* onchange seems to be ontypechange on FxA

Let me know if this looks OK. Thanks!

Comment 33

[Chris Mills [:cmills]]

> * downlinkMax does seem to work on FxA yet, although it does in Chrome

Ooops, I meant "...does NOT seem to work on FxA..."