navigator.connection not exposed in Workers

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: stefhak, Assigned: baku)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

50 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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

Updated

2 years ago
Component: Untriaged → DOM
Product: Firefox → Core
This affects all worker types and not just service workers.
Status: UNCONFIRMED → NEW
Component: DOM → DOM: Workers
Ever confirmed: true
Summary: navigator.connection not exposed to Service Worker → navigator.connection not exposed in Workers
baku may be interested in taking this.
Flags: needinfo?(amarchesini)
Priority: -- → P3
(Assignee)

Updated

2 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
(Assignee)

Comment 3

2 years ago
Attachment #8818569 - Flags: review?(bkelly)
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.
Flags: needinfo?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8818569 - Attachment description: networkInfo.patch → part 1 - workers
Flags: needinfo?(amarchesini)
(Assignee)

Comment 5

2 years ago
Posted patch part 2 - spec (obsolete) — Splinter Review
Here a second patch for the spec diff.
We don't have downlinkMax info (nor connectionType actually).
Attachment #8818577 - Flags: review?(bkelly)
(Assignee)

Comment 6

2 years ago
Posted patch part 2 - specSplinter Review
Attachment #8818577 - Attachment is obsolete: true
Attachment #8818577 - Flags: review?(bkelly)
Attachment #8818578 - Flags: review?(bkelly)
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.
Attachment #8818578 - Flags: review?(bkelly) → review-
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.
Attachment #8818569 - Flags: review?(bkelly) → review+
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 10

2 years ago
> > +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
(Assignee)

Updated

2 years ago
Blocks: 1323658

Comment 11

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f305096ee45
Expose NetworkInformation interface to workers, r=bkelly
sorry had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=40629921&repo=mozilla-inbound
Flags: needinfo?(amarchesini)

Comment 13

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5d50943bbc
Backed out changeset 8f305096ee45 for crashes in /test_tcpsocket_enabled_no_perm.html

Comment 14

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7638db235fd
Expose NetworkInformation interface to workers, r=bkelly
(Assignee)

Updated

2 years ago
Flags: needinfo?(amarchesini)
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!
Flags: needinfo?(amarchesini)

Comment 16

2 years ago
Backout by cbook@mozilla.com:
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
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 18

2 years ago
> 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 :)
Flags: needinfo?(amarchesini)
(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

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaeb9bcf954a
Expose NetworkInformation interface to workers, r=bkelly

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aaeb9bcf954a
https://hg.mozilla.org/mozilla-central/rev/75cb10a1f69f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 23

2 years ago
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.  :(
Flags: needinfo?(amarchesini)
(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

2 years ago
(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?
> 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.
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)

Comment 27

2 years ago
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.
(Assignee)

Comment 28

2 years ago
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 :)
Flags: needinfo?(amarchesini)
(Reporter)

Comment 29

2 years ago
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.
(Reporter)

Comment 30

2 years ago
BTW, how do I enable if I'd like to test on a desktop version of Nightly?
(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)
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!
> * downlinkMax does seem to work on FxA yet, although it does in Chrome

Ooops, I meant "...does NOT seem to work on FxA..."
You need to log in before you can comment on or make changes to this bug.