868913 - [Buri][Tethering]PC can't display RNDIS com port when reset handset

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P1)

Description

[sync-1]

SW125
 AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.080
 Firefox os  v1.0.1
 Mozilla build ID:20130418070206
 
 +++ This bug was initially created as a clone of Bug #446010 +++
 
 DEFECT DESCRIPTION:
     PC can't display RNDIS com port when reset handset    
 
  REPRODUCING PROCEDURES:
     1. Connect handset with PC 
     2. Open USB tethering on handset
     3. PC can pop-up RNDIS com port---OK
     4. Reset handset and check USB tethering setting item, this item was open.
     5. Open device manager on PC, didn't find RNDIS com port---KO
 
  EXPECTED BEHAVIOUR:
     When reset handset, the USB tethering item can auto close
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:100%,3/3
 
  For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #446010 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Dear all,
 The "reset" in the REPRODUCING PROCEDURES means restart.

Comment 2

[xiaokang.chen]

Hi,
Actrually, this bug means after restart, the "usb Tethering" shows enabled(both in the status bar and Settings>Internet Sharing), but was not enabled in reality.
BTW, we can enable the usb Tethering while don't connect the handset with pc.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

Vincent, any comments? thanks

Comment 4

[Shian-Yow Wu [:swu]]

Yes, it's a known issue.

Assigned to Vincent.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Attachment: WIP

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

1. add state queue to handle usb tethering settings request.
2. use handleWifiEnabled function instead of setWifiEnabled, we have state queue mechanism in handleWifiEnabled function. 
3. read the initial settings for usb tethering.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 750384 [details] [diff] [review]
Patch v1.1

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

::: dom/system/gonk/NetworkManager.js
@@ +182,5 @@
> +      if (!aResult) {
> +        return;
> +      }
> +      let settingsLock = gSettingsService.createLock();
> +      settingsLock.set(SETTINGS_WIFI_ENABLED, aResult, null);

This set doesn't do anything other than set the value back to what it was when it was read. What is this code supposed to do?

@@ +640,5 @@
> +    // Pop out the current request.
> +    this._stateRequests.shift();
> +    if (this._stateRequests.length > 0) {
> +      // Handle the latest request.
> +      let latestState = this._stateRequests.pop();

It is really weird to see a shift followed by a pop (I guess the idea is to ignore any intermediate requests). Given that you seem to want to ignore all of the intermediate state change requests, why not keep a single "_lastRequested" variable that's set to the last non-handled state request change and a boolean "_enabling"?

Comment 8

[Vincent Chang[:vchang][changyihsin]]

Thanks for your quick review. 

> > +      let settingsLock = gSettingsService.createLock();
> > +      settingsLock.set(SETTINGS_WIFI_ENABLED, aResult, null);
> 
> This set doesn't do anything other than set the value back to what it was
> when it was read. What is this code supposed to do?

We are listening the "tethering.wifi.enabled" observer event in WifiWorker.js, and I would like to start wifi tethering by settings there. We could move this to WifiWorker.js but I would like to make sure we always read the initial settings for wifi tethering before we start it.  

> @@ +640,5 @@
> > +    // Pop out the current request.
> > +    this._stateRequests.shift();
> > +    if (this._stateRequests.length > 0) {
> > +      // Handle the latest request.
> > +      let latestState = this._stateRequests.pop();
> 
> It is really weird to see a shift followed by a pop (I guess the idea is to
> ignore any intermediate requests). Given that you seem to want to ignore all
> of the intermediate state change requests, why not keep a single
> "_lastRequested" variable that's set to the last non-handled state request
> change and a boolean "_enabling"?
ok, sounds a good idea, one quick question here, do you think we better to keep all the settings requests and handle it later(even if I just care about the last request right now) ? Not quite sure if it is useful information.

Comment 9

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.2

Address the review comments. 
Sorry, I also add the fix to Bug 854353 - [Buri][USB tethering]Computer cann't surf Internet when switch Wi-Fi to mobile data in this patch. :-)

Comment 10

[xiaokang.chen]

(In reply to Vincent Chang[:vchang] from comment #9)
Dear Vincent,
Could you have a look at bug 868902, and give some advice there? Thanks very much.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Vincent Chang[:vchang] from comment #8)
> We are listening the "tethering.wifi.enabled" observer event in
> WifiWorker.js, and I would like to start wifi tethering by settings there.
> We could move this to WifiWorker.js but I would like to make sure we always
> read the initial settings for wifi tethering before we start it.  

Talking to gwagner, doing this during startup is really not a good idea (since a ton of modules are coming up and reading settings). Can you add something to nsIWifi to allow NetworkManager to talk directly to the wifi worker on startup?

> ok, sounds a good idea, one quick question here, do you think we better to
> keep all the settings requests and handle it later(even if I just care about
> the last request right now) ? Not quite sure if it is useful information.

No, I don't think it's worth keeping that information around.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 750975 [details] [diff] [review]
Patch v1.2

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

A couple more questions (in addition to comment 11).

::: dom/system/gonk/NetworkManager.js
@@ +577,5 @@
>      this.tetheringSettings[SETTINGS_USB_DNS1] = DEFAULT_DNS1;
>      this.tetheringSettings[SETTINGS_USB_DNS2] = DEFAULT_DNS2;
>    },
>  
> +  _lastRequested: null,

What are the possible values of _lastRequested? null, and what else?

@@ +638,5 @@
>    // External and internal interface name.
>    _tetheringInterface: null,
>  
> +  handleLastRequest: function handleLastRequest() {
> +    if (this._lastRequested) {

If this._lastRequested is false (as opposed to null), how will this turn off tethering?

Comment 13

[Vincent Chang[:vchang][changyihsin]]

> Talking to gwagner, doing this during startup is really not a good idea
> (since a ton of modules are coming up and reading settings). Can you add
> something to nsIWifi to allow NetworkManager to talk directly to the wifi
> worker on startup?

Maybe I can use an array _wifiTetheringSettingsToRead[a, b, c....] and _usbTetheringSettingsToRead and do _xxxTetheringSettingsToRead.splice() once I read one entry from settings db. Turn on/off the tethering until mandatory settings entries for tethering are read ?

How do you think if I move the wifi tethering settings from NetworkManager to WifiWoker, and add one more parameter data to setWifiTethering API in nsINetworkManager.idl ? 

void setWifiTethering(in boolean enabled,                                               
                      in nsINetworkInterface networkInterface,
                      in jsval data,
                      in nsIWifiTetheringCallback callback);

Comment 14

[Vincent Chang[:vchang][changyihsin]]

> ::: dom/system/gonk/NetworkManager.js
> @@ +577,5 @@
> >      this.tetheringSettings[SETTINGS_USB_DNS1] = DEFAULT_DNS1;
> >      this.tetheringSettings[SETTINGS_USB_DNS2] = DEFAULT_DNS2;
> >    },
> >  
> > +  _lastRequested: null,
> 
> What are the possible values of _lastRequested? null, and what else?
> 
> @@ +638,5 @@
> >    // External and internal interface name.
> >    _tetheringInterface: null,
> >  
> > +  handleLastRequest: function handleLastRequest() {
> > +    if (this._lastRequested) {
> 
> If this._lastRequested is false (as opposed to null), how will this turn off
> tethering?

Good catch, thanks for your reminding.

Maybe I should use one more variable _requestCount to indicate that if we have multiple requests ? So that _lastRequested always indicate the last request and should be true or false. 

BTW, this.wantConnectionEvent should only be called when we have only one request.  The new request always has up to data external interface when connection status of external interface is changed.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Vincent Chang[:vchang] from comment #13)
> Maybe I can use an array _wifiTetheringSettingsToRead[a, b, c....] and
> _usbTetheringSettingsToRead and do _xxxTetheringSettingsToRead.splice() once
> I read one entry from settings db. Turn on/off the tethering until mandatory
> settings entries for tethering are read ?

Sure, that sounds reasonable.

> How do you think if I move the wifi tethering settings from NetworkManager
> to WifiWoker, and add one more parameter data to setWifiTethering API in
> nsINetworkManager.idl ? 

That seems like a fine solution.

(In reply to Vincent Chang[:vchang] from comment #14)
> Maybe I should use one more variable _requestCount to indicate that if we
> have multiple requests ? So that _lastRequested always indicate the last
> request and should be true or false. 

Yep that seems right. You probably don't need a count as much a boolean "pending request" or something like that.

Comment 16

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.3

Comment 17

[sync-1]

Comment from Mozilla:HASH(0xabdffa4)

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 755868 [details] [diff] [review]
Patch v1.3

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

Phew, this was huge. I think it mostly looks good (with the exception of a couple of minor questions below).

::: dom/wifi/WifiWorker.js
@@ -2132,4 @@
>          return;
>        if (aResult === null)
>          aResult = false;
> -      DEBUG = aResult;

Why remove this?

@@ +3086,3 @@
>          break;
> +      case SETTINGS_WIFI_TETHERING_ENABLED:
> +        this._oldWifiTetheringEnabledState = this.tetheringSettings[SETTINGS_WIFI_TETHERING_ENABLED];

Please add an explicit "// Fall through" comment here.

Comment 19

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch ready to commit v1.4

I rebase the patch and also address the comment 18. 
I also found three bugs while rebase and test the patch.
1. The initial settings for wifi hotspot should be ignored if it is set to false.
Because wifi hotspot is already in disable state. 
2. Hook the callback timer to WifiWorker object to assure the timer is fired. Based on the comment in mdn for nsITimer, users of instances of nsITimer should keep a reference to the timer until it is no longer needed in order to assure the timer is fired.
3. the currentNetwork maybe a null value when receive COMPLETED event.  
https://mxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#1928

Comment 20

[Vincent Chang[:vchang][changyihsin]]

Push to birch directly because I have got the review+ in comment 18. 
 
https://hg.mozilla.org/projects/birch/rev/761e842067d8

Comment 21

[Vincent Chang[:vchang][changyihsin]]

This patch also fixed the bug 854353 and should be leo+. Nominate it to leo ?

Comment 23

[Vincent Chang[:vchang][changyihsin]]

Attachment: Part1 - Patch v1.4 for b2g18 branch

I has tried this patch with b2g18 branch. We can land it to b2g17 once we get the leo+.

Comment 24

[Vincent Chang[:vchang][changyihsin]]

Hi Leo, can you try this patch ?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/761e842067d8

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7fbf56d72d34

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/7fbf56d72d34

Comment 28

[Ryan VanderMeulen [:RyanVM]]

This was backed out in bug 879927. It'll need an updated branch patch.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a80a19cf1170
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a80a19cf1170

Comment 29

[Vincent Chang[:vchang][changyihsin]]

Attachment: b2g18 - Part1 - patch ready to commit for b2g18

Remove the trailing white space.

Comment 30

[Vincent Chang[:vchang][changyihsin]]

Attachment: Part2 - patch for b2g18 and fixed initial value bug in part1

This patch fixed the initial start of wifi failed problem. 
The root cause is the value of this._oldWifiTetheringEnabledState may be set to "undefined", and the wifi hotspot off operation may happen in middle of enabling wifi procedure.

Comment 31

[Vincent Chang[:vchang][changyihsin]]

Attachment: Part2 - patch for m-c that fixed initial value for v1.4

After applying v1.4 patch, we may have wifi initial start bug. Somehow it is hard to reproduce in m-c. But we still need to apply this patch to prevent any regression.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Can you please give the follow-up a commit message that isn't the same as the first part?

Comment 33

[Vincent Chang[:vchang][changyihsin]]

Attachment: m-c Part 2 - Patch ready for commit to m-c

Modify the commit message for part 2 patch. Please help to commit this patch to inbound and m-c

Comment 34

[Vincent Chang[:vchang][changyihsin]]

Attachment: b2g18 - part 2 - Patch ready for commit to b2g18

Modify the commit message for b2g18 part 2 patch.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Thanks :). I'll land this on b2g18 once it's merged over to m-c.
https://hg.mozilla.org/projects/birch/rev/2f49ab9a4da4

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f49ab9a4da4

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5a05d489a2ac
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5c13ed3c584c

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5a05d489a2ac
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5c13ed3c584c