1120843 - HTTP proxy setting should be cleared after data call disconnected

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Shian-Yow Wu [:swu]]

Attachment: Console log

Device: Flame
Gecko: b8876c827edb + applied fix for bug 1120302
Gaia: d102cc0a7a1f346531553bec64588eea9e4594eb

STR:
1. Disable wifi
2. Enable data call with http proxy setting
3. Disable data call

Expected:
HTTP proxy setting should be cleared from preference.

Actual:
HTTP proxy setting is not cleared from preference.

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: Console log

Sorry, this one is the correct log.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Thanks for reporting this, Shian-Yow. You are right, we are not clearing http proxy for data call nor wifi, when they get disconnected. I'll add this part.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

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

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 8548667 [details] [diff] [review]
patch, v1.

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

Looks good, but could you also help to add a test case for proxy?
Maybe use `SpecialPowers.get*Pref` to check the proxy configuration.

Thank you.

::: dom/system/gonk/NetworkService.js
@@ +377,5 @@
>    setNetworkProxy: function(network) {
>      try {
>        if (!network.httpProxyHost || network.httpProxyHost === "") {
>          // Sets direct connection to internet.
> +        this.clearNetworkProxy();

I'd like to move these proxy code to NetworkManager and let NetworkManager set/clear pref directly. But I found wifi will use |NetworkService.setNetworkProxy| as well. Let's just keep current architecture now and see if it is necessary to come out a generic flow for both wifi and data later. Thank you.

@@ +400,5 @@
>      }
>    },
>  
> +  clearNetworkProxy: function() {
> +    if(DEBUG) debug("Going to clear all network proxy.");

nit: space between `if` and `(`.

Comment 5

[Henry Chang [:hchang]]

(In reply to Edgar Chen [:edgar][:echen] from comment #4)
> ::: dom/system/gonk/NetworkService.js
> @@ +377,5 @@
> >    setNetworkProxy: function(network) {
> >      try {
> >        if (!network.httpProxyHost || network.httpProxyHost === "") {
> >          // Sets direct connection to internet.
> > +        this.clearNetworkProxy();
> 
> I'd like to move these proxy code to NetworkManager and let NetworkManager
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> set/clear pref directly. 

I couldn't agree with this anymore :p 

I am also aware of this when fixing Bug 1120302. After blaming the code 
I found I was the one who put it to NetworkService >"< 
Will file a bug to address this.

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Henry Chang [:henry] from comment #5)
> (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > ::: dom/system/gonk/NetworkService.js
> > @@ +377,5 @@
> > >    setNetworkProxy: function(network) {
> > >      try {
> > >        if (!network.httpProxyHost || network.httpProxyHost === "") {
> > >          // Sets direct connection to internet.
> > > +        this.clearNetworkProxy();
> > 
> > I'd like to move these proxy code to NetworkManager and let NetworkManager
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > set/clear pref directly. 
> 
> I couldn't agree with this anymore :p 
> 
> I am also aware of this when fixing Bug 1120302. After blaming the code 
> I found I was the one who put it to NetworkService >"< 
> Will file a bug to address this.

Thanks! btw, what is wifi's |configureHttpProxy| for? Will someone call this function explicitly? If not, maybe we should let NetworkManager handle the proxy stuff.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: clear http proxy when active network is disconnected.

Rebased and fix possible null exception in |this.active|.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: test cases for data connection http proxy.

Edgar, may I have your review on the test case? Thanks.

Comment 9

[Jessica Jong [:jessica] (inactive)]

We found that it is possible that 'network-connection-state-change' event is fired before routing/http proxy/dnses are set correctly. This may lead to unexpected results, we should file a bug to handle that.

Comment 10

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #9)
> We found that it is possible that 'network-connection-state-change' event is
> fired before routing/http proxy/dnses are set correctly. This may lead to
> unexpected results, we should file a bug to handle that.

filed bug 1126222 for this.

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8555137 [details] [diff] [review]
Part 2: test cases for data connection http proxy.

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

Thanks for working on the test case, please see my comments below. :)

::: dom/system/gonk/tests/marionette/test_data_connection_proxy.js
@@ +20,5 @@
> +}
> +
> +function setTestApn() {
> +  let apn = [
> +    [ {"carrier":"T-Mobile US",

nit: space after `"`, here and below.

@@ +31,5 @@
> +
> +  return setSettings(SETTINGS_KEY_DATA_APN_SETTINGS, apn);
> +}
> +
> +function waitForTimeout(aTimeoutMs) {

I guess you could just simply use `waitFor` [1] then we don't need this function.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Marionette_JavaScript_Tests#waitFor%28%29

@@ +32,5 @@
> +  return setSettings(SETTINGS_KEY_DATA_APN_SETTINGS, apn);
> +}
> +
> +function waitForTimeout(aTimeoutMs) {
> +  let deferred = Promise.defer();

`Deferred` is obsolete since Gecko 30 [2], please use `new Promise` instead.

e.g.

return new Promise(function(aResolve, aReject) {
  setTimeout(function() {
    aResolve();
  }, aTimeoutMs);
});

[2] https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Deferred

@@ +42,5 @@
> +  return deferred.promise;
> +}
> +
> +function verifyHttpProxy(aIsSet) {
> +  let deferred = Promise.defer();

Ditto

@@ +82,5 @@
> +                                    "network.proxy.ssl_port");
> +      }
> +    }, () => {
> +       if (!retryCnt--) {
> +         throw "Failed waiting for http proxy.";

Looks like we won't have any chance to run into this condition given that the |retryCnt| always starts with `20`?

And how about rewriting with `waitFor` to make it simpler though I am not sure if this can totally cover your original idea?

e.g.

function waitForHttpProxyVerified(aShouldBeSet) {
  return new Promise(function(aResolve, aReject) {
    try {
      waitFor(aResolve, () => {
        ....
        // Check proxy value here.
      }, TIME_OUT_VALUE);
    } catch() {
      // Time out.
      aReject();
    }
  });
}

Comment 12

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #11)
> Comment on attachment 8555137 [details] [diff] [review]
> Part 2: test cases for data connection http proxy.
> 
> Review of attachment 8555137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for working on the test case, please see my comments below. :)
> 
> ::: dom/system/gonk/tests/marionette/test_data_connection_proxy.js
> @@ +20,5 @@
> > +}
> > +
> > +function setTestApn() {
> > +  let apn = [
> > +    [ {"carrier":"T-Mobile US",
> 
> nit: space after `"`, here and below.
                   ^^^^
                   Sorry, it should be `:`. :p

Comment 13

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #11)
> Comment on attachment 8555137 [details] [diff] [review]
> Part 2: test cases for data connection http proxy.
> 
> Review of attachment 8555137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for working on the test case, please see my comments below. :)
> 
> ::: dom/system/gonk/tests/marionette/test_data_connection_proxy.js
> @@ +20,5 @@
> > +}
> > +
> > +function setTestApn() {
> > +  let apn = [
> > +    [ {"carrier":"T-Mobile US",
> 
> nit: space after `"`, here and below.
> 
> @@ +31,5 @@
> > +
> > +  return setSettings(SETTINGS_KEY_DATA_APN_SETTINGS, apn);
> > +}
> > +
> > +function waitForTimeout(aTimeoutMs) {
> 
> I guess you could just simply use `waitFor` [1] then we don't need this
> function.
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/
> Marionette_JavaScript_Tests#waitFor%28%29
> 
> @@ +32,5 @@
> > +  return setSettings(SETTINGS_KEY_DATA_APN_SETTINGS, apn);
> > +}
> > +
> > +function waitForTimeout(aTimeoutMs) {
> > +  let deferred = Promise.defer();
> 
> `Deferred` is obsolete since Gecko 30 [2], please use `new Promise` instead.
> 
> e.g.
> 
> return new Promise(function(aResolve, aReject) {
>   setTimeout(function() {
>     aResolve();
>   }, aTimeoutMs);
> });
> 
> [2]
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> Promise.jsm/Deferred
> 
> @@ +42,5 @@
> > +  return deferred.promise;
> > +}
> > +
> > +function verifyHttpProxy(aIsSet) {
> > +  let deferred = Promise.defer();
> 
> Ditto
> 
> @@ +82,5 @@
> > +                                    "network.proxy.ssl_port");
> > +      }
> > +    }, () => {
> > +       if (!retryCnt--) {
> > +         throw "Failed waiting for http proxy.";
> 
> Looks like we won't have any chance to run into this condition given that
> the |retryCnt| always starts with `20`?
> 
> And how about rewriting with `waitFor` to make it simpler though I am not
> sure if this can totally cover your original idea?
> 
> e.g.
> 
> function waitForHttpProxyVerified(aShouldBeSet) {
>   return new Promise(function(aResolve, aReject) {
>     try {
>       waitFor(aResolve, () => {
>         ....
>         // Check proxy value here.
>       }, TIME_OUT_VALUE);
>     } catch() {
>       // Time out.
>       aReject();
>     }
>   });
> }

Thanks for the helpful comments! I'll upload another patch base on these comments.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: test cases for data connection http proxy, v2.

Edgar, I have revised the patch according to review comment 11, may I have your review again? Thanks.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8556937 [details] [diff] [review]
Part 2: test cases for data connection http proxy, v2.

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

Really nice work, thank you.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Thanks Edgar.

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cceb8440177

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/17d151a8dee1
https://hg.mozilla.org/integration/b2g-inbound/rev/9a14a438cde6

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17d151a8dee1
https://hg.mozilla.org/mozilla-central/rev/9a14a438cde6

Comment 19

[Edgar Chen [:edgar]]

(In reply to Henry Chang [:henry] from comment #5)
> (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > ::: dom/system/gonk/NetworkService.js
> > @@ +377,5 @@
> > >    setNetworkProxy: function(network) {
> > >      try {
> > >        if (!network.httpProxyHost || network.httpProxyHost === "") {
> > >          // Sets direct connection to internet.
> > > +        this.clearNetworkProxy();
> > 
> > I'd like to move these proxy code to NetworkManager and let NetworkManager
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > set/clear pref directly. 
> 
> I couldn't agree with this anymore :p 
> 
> I am also aware of this when fixing Bug 1120302. After blaming the code 
> I found I was the one who put it to NetworkService >"< 
> Will file a bug to address this.

Hi Henry, have you already filed a bug for this?

Comment 20

[Henry Chang [:hchang]]

(In reply to Edgar Chen [:edgar][:echen] from comment #19)
> (In reply to Henry Chang [:henry] from comment #5)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > > ::: dom/system/gonk/NetworkService.js
> > > @@ +377,5 @@
> > > >    setNetworkProxy: function(network) {
> > > >      try {
> > > >        if (!network.httpProxyHost || network.httpProxyHost === "") {
> > > >          // Sets direct connection to internet.
> > > > +        this.clearNetworkProxy();
> > > 
> > > I'd like to move these proxy code to NetworkManager and let NetworkManager
> >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > set/clear pref directly. 
> > 
> > I couldn't agree with this anymore :p 
> > 
> > I am also aware of this when fixing Bug 1120302. After blaming the code 
> > I found I was the one who put it to NetworkService >"< 
> > Will file a bug to address this.
> 
> Hi Henry, have you already filed a bug for this?

Just filed! Bug 1130962. Thanks for reminding!

Comment 21

[Edgar Chen [:edgar]]

(In reply to Henry Chang [:henry] from comment #20)
> (In reply to Edgar Chen [:edgar][:echen] from comment #19)
> > (In reply to Henry Chang [:henry] from comment #5)
> > > (In reply to Edgar Chen [:edgar][:echen] from comment #4)
> > > > ::: dom/system/gonk/NetworkService.js
> > > > @@ +377,5 @@
> > > > >    setNetworkProxy: function(network) {
> > > > >      try {
> > > > >        if (!network.httpProxyHost || network.httpProxyHost === "") {
> > > > >          // Sets direct connection to internet.
> > > > > +        this.clearNetworkProxy();
> > > > 
> > > > I'd like to move these proxy code to NetworkManager and let NetworkManager
> > >   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > set/clear pref directly. 
> > > 
> > > I couldn't agree with this anymore :p 
> > > 
> > > I am also aware of this when fixing Bug 1120302. After blaming the code 
> > > I found I was the one who put it to NetworkService >"< 
> > > Will file a bug to address this.
> > 
> > Hi Henry, have you already filed a bug for this?
> 
> Just filed! Bug 1130962. Thanks for reminding!

Thank you, Henry.