1126222 - B2G NetworkManager: fire 'network-connection-state-changed' after internal work is done

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Jessica Jong [:jessica] (inactive)]

We should fire 'network-connection-state-changed' after routes/dnses/proxy are set/cleared. Maybe use promise in this case.

Comment 1

[Bhavna Sharma]

Location components rely a lot on this topic change network-connection-state-changed, to trigger pending time, xtra data downloads or trigger pending crowdsourcing uploads. Getting this notification must mean that the routes/dns/proxies are all setup and ready for use immediately.

Comment 2

[Ken Chang[:kenkai]]

Edgar, This bug seems a refactor. Does it create any critical issue that user will face?

Comment 3

[Kai-Chih Hu [:khu]]

Is it too late to include this into the CAF bug list? Thanks.

Comment 4

[Edgar Chen [:edgar]]

(In reply to Ken Chang[:ken] from comment #2)
> Edgar, This bug seems a refactor. Does it create any critical issue that
> user will face?

In current design, there is a very short period that user possibly unable to access network after receiving 'network-connection-state-changed' due to the internal setup (routing, proxy ... etc) was not ready yet. This bug is trying to improve the design, though we haven't received any bug that is caused by it.

Comment 5

[Mike Lee [:mlee]]

Anshul,

Please work with Bhavna Sharma to clarify whether this needs to be a CAF FC blocker.

Thanks,
Mike

Comment 6

[pgravel]

This is causing an issue with MMS.

>2078:04-08 15:09:52.772   261   261 I Gecko   : -@- MmsService: Failed to ensureRouting: Error: Failed to resolve 'proxy.mobile.att.net', with status: 2152398878
>2079:04-08 15:09:52.782   261   261 I Gecko   : -@- MmsService: The sending status of sendTransaction.run(): -8
>2080:04-08 15:09:52.782   261   261 I Gecko   : -@- MmsService: The returned status of sending transaction: aErrorCode: 4 aEnvelopeId: null
>2089:04-08 15:09:52.802   261  2405 D NetworkUtils: Preparing to send 'resolver setnetdns 100 mozilla.rmnet_data0.doman 172.26.38.1 172.26.38.2' command...
>2090:04-08 15:09:52.802   261  2405 D NetworkUtils: Sending '0 resolver setnetdns 100 mozilla.rmnet_data0.doman 172.26.38.1 172.26.38.2' command to netd.
>2091:04-08 15:09:52.802   261  2405 D NetworkUtils: Receiving "0 resolver setnetdns 100 mozilla.rmnet_data0.doman 172.26.38.1 172.26.38.2" command response from netd.
>2092:04-08 15:09:52.802   261  2405 D NetworkUtils:           ==> Code: 200  Reason: Resolver command succeeded


This happens somewhat consistently when trying to send an MMS with no data call is currently up.

Comment 7

[Ken Chang[:kenkai]]

When the case mentioned in comment 6 happens, won't MMS service try to establish data call?

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Ken Chang[:ken] from comment #7)
> When the case mentioned in comment 6 happens, won't MMS service try to
> establish data call?

No, The data call is established before this error was thrown and the connection is reference-counted, so it will be kept alive unless disconnected by network/modem.

Comment 9

[Bhavna Sharma]

Move needinfo to Phil, as he can provide more information on MMS service.

Comment 10

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

NI myself to track this.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

This seems to be covered by the change in NetworkManager of the patch in bug 992772.
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=992772&attachment=8592666
setDNS() was wrapped as Promise and 
'network-connection-state-changed' will be fired after setDNS() is done.

Comment 12

[Jessica Jong [:jessica] (inactive)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #11)
> This seems to be covered by the change in NetworkManager of the patch in bug
> 992772.
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=992772&attachment=8592666
> setDNS() was wrapped as Promise and 
> 'network-connection-state-changed' will be fired after setDNS() is done.

Actually, it is not completely covered: 'network-connection-state-changed' will be fired after setDNS() is done, but it is not guaranteed it will be fired after dns routes are set. So, if there is no existing default route, there is a chance that dns routes are not ready when 'network-connection-state-changed' is fired.

However, this has always been this way and we haven't had issues before. I'd like to see if the mms issue still exists after bug 1152568.

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Phil,
Comment 6 is not obviously pointing to the root cause being this bug. It smells more like bug 1152568 instead. We need more logs from you if you still think the 'network-connection-state-changed' firing timing brings you some trouble in mms sending.

Comment 14

[Ken Chang[:kenkai]]

Michael, can we remove this bug from 2.2 CAF list and focus on bug 1152568?

Comment 15

[pgravel]

Attachment: mmfailusingdefaultdata.log

Apologies for the delay, got sick and have been scrambling to catch up on various issues.

I still believe this is a different issue than 1152568.

I'm attaching logs of my test scenario with full debug enabled. I added a couple of logs to help track network-connection-state-changed notifications and you'll probably also notice the two lines with "PHIL" in the logs which denotes very small tweaks[1][2] I've made in MmsService which I've made in order to fully isolate this issue from 1152568.


Steps followed in the attached log (mmfailusingdefaultdata.log)
1. reboot
2. Have data down
3. Try to send a MMS
4. MMS will fail to send
5. Immediately re-send the same MMS by pressing the (!) mark and pressing OK
6. MMS will send successfully


The 2nd attempt at sending the MMS is done within the time window that the MMS connection remains active, so the exact same data call is used and no new NetworkInterface is registered or modified within the NetworkManager. Since the 2nd attempt succeeds while there is no apparent changes to the connection, i still feels like we are running into the aforementioned timing problem with dns.


[1] - SetupDataCallByType('mms') results in both NETWORK_TYPE_MOBILE and NETWORK_TYPE_MOBILE_MMS network interfaces to be brought up (shared APN, same data call). This is done to avoid running into proxy resolve issues discussed in 1152568 when only NETWORK_TYPE_MOBILE_MMS is registered.
[2] - Made MmsService wait for network-connection-state-changed on NETWORK_TYPE_MOBILE instead of NETWORK_TYPE_MOBILE_MMS because I noticed network-connection-state-changed for MMS comes in before MOBILE since there is less routing to do. This was done to attempt to give MORE time for all netd commands to be sent and resolved, but in the end the behavior is the same and the MMS still fails to resolve the proxy. Logs are pretty much identical either way.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Phil, thanks for the experiments and logs. I think this problem has become more apparent in Lollipop, because all the routes are set asynchronously through netd, whereas before Lollipop, most of them were set using native functions (ifc_*).

However, I have tested myself using Nexus-5-L, and mms can be sent successfully on the first time without default data. I think the difference is because, without bug 1152568, you need to tweak MmsService to wait for NETWORK_TYPE_MOBILE event, which has more routing to do after sending network-connection-state-changed, so it is more likely for NETWORK_TYPE_MOBILE not to be ready when MmsService receives the event. Have you tested this with the patch in bug 1152568 (actually, you need only bug 992772)? so you don't need to tweak MmsService.

I know this is still a problem, so, we'll try to fix this. In the meantime, could you test this with the patch in bug 992772, if it works, maybe this should not be a blocker? Thanks!

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

NI for comment 16, thanks.

Comment 18

[pgravel]

Hi Jessica,

In my last set of logs, I already made the change to wait for NETWORK_TYPE_MOBILE (as mentioned in the [2] note) and it didn't seem to help.

Also, I don't see a patch available for bug 1152568?

I'll make some time tomorrow to try out the patch from bug 992772 and let you know.

Thanks.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Hi Phil, please find my comments below.

(In reply to pgravel from comment #18)
> Hi Jessica,
> 
> In my last set of logs, I already made the change to wait for
> NETWORK_TYPE_MOBILE (as mentioned in the [2] note) and it didn't seem to
> help.

What I meant in comment 16, is that, I _don't_ want MmsService to wait for NETWORK_TYPE_MOBILE, cause it is more vulnerable to race conditions (it has more tasks to do). So, with the patch in bug 992772, you don't need the tweaks.

> 
> Also, I don't see a patch available for bug 1152568?
> 
> I'll make some time tomorrow to try out the patch from bug 992772 and let
> you know.

Yes, please use the patch in bug 992772. Thanks.

> 
> Thanks.

Comment 20

[pgravel]

Hi Jessica,

With the patch from bug 992772 I am able to send MMS much more consistently. I did run into a few failed attempts, but haven't been able to reproduce with any consistency.

So in terms of MMS, I think we're OK.

Comment 21

[Jessica Jong [:jessica] (inactive)]

(In reply to pgravel from comment #20)
> Hi Jessica,
> 
> With the patch from bug 992772 I am able to send MMS much more consistently.
> I did run into a few failed attempts, but haven't been able to reproduce
> with any consistency.
> 
> So in terms of MMS, I think we're OK.

Thanks Phil, that's good news.
Just letting you know, we are also working on this bug.

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to pgravel from comment #20)
> Hi Jessica,
> 
> With the patch from bug 992772 I am able to send MMS much more consistently.
> I did run into a few failed attempts, but haven't been able to reproduce
> with any consistency.
> 
> So in terms of MMS, I think we're OK.

Thanks Phil for the testing.

According to CAF test result, it looks this could be removed from a CAF-v2.2-meta, right?

Comment 23

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: provide callback in some functions in NetworkService.

Comment 24

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: provide callback in some functions in NetworkService.

Comment 25

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: use promise to ensure execution order in NetworkManager.

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Clear NI as Phil already adjusted the bug dependency.

Comment 27

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: use promise to ensure execution order in NetworkManager, v2.

I found that sometimes network info are cleared before NetworkManager finishes its actions, so I let NetworkManager keep a copy of network when updateNetworkInterface() is called.

Comment 28

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8599076 [details] [diff] [review]
Part 1: provide callback in some functions in NetworkService.

Edgar, may I have your review on this? Thanks.

Comment 29

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hey, why is this blocking CAF-2.2 again?

Comment 30

[Dylan Oliver [:doliver]]

ni? to Greg for comment 29.

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8599783 [details] [diff] [review]
Part 2: use promise to ensure execution order in NetworkManager, v2.

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

Mostly looks good, but I would like to see a revised version again, thank you.

::: dom/system/gonk/NetworkManager.js
@@ +100,5 @@
> +  this.httpProxyHost = aNetwork.httpProxyHost;
> +  this.httpProxyPort = aNetwork.httpProxyPort;
> +}
> +NetworkInterface.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsINetworkInterface]),

|NetworkInterface| is only used for caching status, don't need this, unless it will be exposed to other module.

@@ +343,3 @@
>              // Add host route for data calls
> +            if (!this.isNetworkTypeMobile(networkInterface.type)) {
> +              return Promise.resolve();

Just `return;` is enough. The new promise is fulfilled if the callback returns a value not a promise.

@@ +360,5 @@
> +          })
> +          .then(() => {
> +            if (networkInterface.type !=
> +                Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN) {
> +              return Promise.resolve();

Ditto, return;

@@ +388,5 @@
>              Services.obs.notifyObservers(network, TOPIC_CONNECTION_STATE_CHANGED,
>                                           this.convertConnectionType(network));
> +          })
> +          .catch(aError => {
> +            throw new Error(aError);

Catch a rejected promise and create a new rejected promise by throwing an exception?
I guess you could just print some message here instead?

@@ +395,5 @@
>        case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED:
> +        Promise.resolve()
> +          .then(() => {
> +            if (!this.isNetworkTypeMobile(networkInterface.type)) {
> +              return Promise.resolve();

Ditto, return;

@@ +403,5 @@
> +          })
> +          .then(() => {
> +            if (networkInterface.type !=
> +                Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN) {
> +              return Promise.resolve();

Ditto, return;

@@ +446,5 @@
> +            Services.obs.notifyObservers(network, TOPIC_CONNECTION_STATE_CHANGED,
> +                                         this.convertConnectionType(network));
> +          })
> +          .catch(aError => {
> +            throw new Error(aError);

Ditto, print message instead?

@@ +676,5 @@
>      return null;
>    },
>  
> +  _setSecondaryRoute: function(doAdd, interfaceName, route) {
> +    return new Promise((aResolve, aReject) => {

Have a consistent style, put `a` prefix for other arguments.

@@ +678,5 @@
>  
> +  _setSecondaryRoute: function(doAdd, interfaceName, route) {
> +    return new Promise((aResolve, aReject) => {
> +      if (doAdd) {
> +        gNetworkService.addSecondaryRoute(interfaceName, route, (success) => {

s/success/aSuccess/

@@ +815,5 @@
> +    return Promise.resolve()
> +      .then(() => {
> +        // Don't set default route on secondary APN
> +        if (!this.active ||
> +            (this.active && this.isNetworkTypeSecondaryMobile(this.active.type))) {

if(!(this.active && this.isNetworkTypeSecondaryMobile(this.active.type))) {

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2837,2 @@
>          }
> +      }.bind(this, RIL.GECKO_NETWORK_STATE_DISCONNECTED), Ci.nsIEventTarget.DISPATCH_NORMAL);

Nit: Use arrow function instead.

Comment 32

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar for the review.

(In reply to Edgar Chen [:edgar][:echen] from comment #31)
> Comment on attachment 8599783 [details] [diff] [review]
> Part 2: use promise to ensure execution order in NetworkManager, v2.
> 
> Review of attachment 8599783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks good, but I would like to see a revised version again, thank
> you.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +100,5 @@
> > +  this.httpProxyHost = aNetwork.httpProxyHost;
> > +  this.httpProxyPort = aNetwork.httpProxyPort;
> > +}
> > +NetworkInterface.prototype = {
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsINetworkInterface]),
> 
> |NetworkInterface| is only used for caching status, don't need this, unless
> it will be exposed to other module.

Oh, I was thinking whether to use 'networkInterface' or 'network' when calling notifyObservers in updateNetworkInterface(), then I decided to use the original 'network' so I think we don't need this. Thanks.

> 
> @@ +343,3 @@
> >              // Add host route for data calls
> > +            if (!this.isNetworkTypeMobile(networkInterface.type)) {
> > +              return Promise.resolve();
> 
> Just `return;` is enough. The new promise is fulfilled if the callback
> returns a value not a promise.

Got it. Thanks.

> 
> @@ +360,5 @@
> > +          })
> > +          .then(() => {
> > +            if (networkInterface.type !=
> > +                Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN) {
> > +              return Promise.resolve();
> 
> Ditto, return;

Will do.

> 
> @@ +388,5 @@
> >              Services.obs.notifyObservers(network, TOPIC_CONNECTION_STATE_CHANGED,
> >                                           this.convertConnectionType(network));
> > +          })
> > +          .catch(aError => {
> > +            throw new Error(aError);
> 
> Catch a rejected promise and create a new rejected promise by throwing an
> exception?
> I guess you could just print some message here instead?

This is because updateNetworkInterface() does not return a promise, so I think caller does not have the chance to handle the rejected promise, that's why I throw an error instead. Does that make sense?
However, before this patch, we don't throw anything on error, so maybe we should keep the same behaviour.

> 
> @@ +395,5 @@
> >        case Ci.nsINetworkInterface.NETWORK_STATE_DISCONNECTED:
> > +        Promise.resolve()
> > +          .then(() => {
> > +            if (!this.isNetworkTypeMobile(networkInterface.type)) {
> > +              return Promise.resolve();
> 
> Ditto, return;

Will do.

> 
> @@ +403,5 @@
> > +          })
> > +          .then(() => {
> > +            if (networkInterface.type !=
> > +                Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_DUN) {
> > +              return Promise.resolve();
> 
> Ditto, return;

Will do.

> 
> @@ +446,5 @@
> > +            Services.obs.notifyObservers(network, TOPIC_CONNECTION_STATE_CHANGED,
> > +                                         this.convertConnectionType(network));
> > +          })
> > +          .catch(aError => {
> > +            throw new Error(aError);
> 
> Ditto, print message instead?
> 
> @@ +676,5 @@
> >      return null;
> >    },
> >  
> > +  _setSecondaryRoute: function(doAdd, interfaceName, route) {
> > +    return new Promise((aResolve, aReject) => {
> 
> Have a consistent style, put `a` prefix for other arguments.

Will do.

> 
> @@ +678,5 @@
> >  
> > +  _setSecondaryRoute: function(doAdd, interfaceName, route) {
> > +    return new Promise((aResolve, aReject) => {
> > +      if (doAdd) {
> > +        gNetworkService.addSecondaryRoute(interfaceName, route, (success) => {
> 
> s/success/aSuccess/

Will do.

> 
> @@ +815,5 @@
> > +    return Promise.resolve()
> > +      .then(() => {
> > +        // Don't set default route on secondary APN
> > +        if (!this.active ||
> > +            (this.active && this.isNetworkTypeSecondaryMobile(this.active.type))) {
> 
> if(!(this.active && this.isNetworkTypeSecondaryMobile(this.active.type))) {

Hmm, I think this will cover the case where this.active is defined but it is not type secondary mobile, but we don't it to be covered because we need to set default route in this case.

> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +2837,2 @@
> >          }
> > +      }.bind(this, RIL.GECKO_NETWORK_STATE_DISCONNECTED), Ci.nsIEventTarget.DISPATCH_NORMAL);
> 
> Nit: Use arrow function instead.

Will do.

Comment 33

[Edgar Chen [:edgar]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #32)
> (In reply to Edgar Chen [:edgar][:echen] from comment #31)
> > ::: dom/system/gonk/NetworkManager.js
> > @@ +388,5 @@
> > >              Services.obs.notifyObservers(network, TOPIC_CONNECTION_STATE_CHANGED,
> > >                                           this.convertConnectionType(network));
> > > +          })
> > > +          .catch(aError => {
> > > +            throw new Error(aError);
> > 
> > Catch a rejected promise and create a new rejected promise by throwing an
> > exception?
> > I guess you could just print some message here instead?
> 
> This is because updateNetworkInterface() does not return a promise, so I
> think caller does not have the chance to handle the rejected promise, that's
> why I throw an error instead. Does that make sense?

Throwing an exception in the promise callback doesn't really "throw an exception", but create a new rejected promise with the exception as the rejection reason [1].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#then%28%29 

> However, before this patch, we don't throw anything on error, so maybe we
> should keep the same behaviour.

Comment 34

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #33)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #32)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #31)
> > > ::: dom/system/gonk/NetworkManager.js
> > > @@ +388,5 @@
> > > >              Services.obs.notifyObservers(network, TOPIC_CONNECTION_STATE_CHANGED,
> > > >                                           this.convertConnectionType(network));
> > > > +          })
> > > > +          .catch(aError => {
> > > > +            throw new Error(aError);
> > > 
> > > Catch a rejected promise and create a new rejected promise by throwing an
> > > exception?
> > > I guess you could just print some message here instead?
> > 
> > This is because updateNetworkInterface() does not return a promise, so I
> > think caller does not have the chance to handle the rejected promise, that's
> > why I throw an error instead. Does that make sense?
> 
> Throwing an exception in the promise callback doesn't really "throw an
> exception", but create a new rejected promise with the exception as the
> rejection reason [1].
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> Promise.jsm/Promise#then%28%29 

I get what you mean now, thanks for the information, I'll just print some error message here.

> 
> > However, before this patch, we don't throw anything on error, so maybe we
> > should keep the same behaviour.

Comment 35

[Greg Grisco]

Removing this from CAF-2.2 blockers again.  It shouldn't come back unintentionally this time.

Comment 36

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: provide callback in some functions in NetworkService. r=echen

Add r=echen

Comment 37

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: use promise to ensure execution order in NetworkManager, v3.

Hi Edgar, I have addressed the review comments in comment 31. Could you take a look again? Thanks.

Note that I kept the QueryInterface part in NetworkInterface cause it is exposed to NetworkService.

Comment 38

[Edgar Chen [:edgar]]

Comment on attachment 8601316 [details] [diff] [review]
Part 2: use promise to ensure execution order in NetworkManager, v3.

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

Looks good, thank you.

::: dom/system/gonk/NetworkManager.js
@@ +100,5 @@
> +  this.httpProxyHost = aNetwork.httpProxyHost;
> +  this.httpProxyPort = aNetwork.httpProxyPort;
> +}
> +NetworkInterface.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsINetworkInterface]),

(In reply to Jessica Jong [:jjong] [:jessica] from comment #37)
> Note that I kept the QueryInterface part in NetworkInterface cause it is
> exposed to NetworkService.

I see. I missed NetworkService.
We are also considering don't take the nsINetworkInterface directly as argument in NetworkService, but spread the arguments up.

@@ +109,5 @@
> +
> +    return this.ips.length;
> +  },
> +
> +  getGateways: function (aCount) {

Nit: Remove space between 'function' and '('.

@@ +116,5 @@
> +    }
> +    return this.gateways.slice();
> +  },
> +
> +  getDnses: function (aCount) {

Nit: Ditto.

Comment 39

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: use promise to ensure execution order in NetworkManager. r=echen

Thanks Edgar!

Comment 40

[Jessica Jong [:jessica] (inactive)]

try looks good but b2g-inbound is closed at the moment.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cc33c7fa13f

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d210ab7b17ef
https://hg.mozilla.org/integration/b2g-inbound/rev/772d976eb3e8

Comment 42

[Jessica Jong [:jessica] (inactive)]

Patches were merged into m-c last week.

https://hg.mozilla.org/mozilla-central/rev/d210ab7b17ef
https://hg.mozilla.org/mozilla-central/rev/772d976eb3e8