Closed Bug 868913 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE3 (26jun)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: sync-1, Assigned: vchang)

References

Details

(Whiteboard: RN5/29)

Attachments

(4 files, 7 obsolete files)

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:
Dear all,
 The "reset" in the REPRODUCING PROCEDURES means restart.
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.
Flags: needinfo?(swu)
Flags: needinfo?(jcheng)
Vincent, any comments? thanks
Flags: needinfo?(vchang)
Flags: needinfo?(swu)
Flags: needinfo?(jcheng)
Yes, it's a known issue.

Assigned to Vincent.
Assignee: nobody → vchang
Flags: needinfo?(vchang)
Attached patch WIP (obsolete) — Splinter Review
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #748752 - Attachment is obsolete: true
Attachment #750384 - Flags: review?(mrbkap)
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"?
Attachment #750384 - Flags: review?(mrbkap)
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.
Attached patch Patch v1.2 (obsolete) — Splinter Review
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. :-)
Attachment #750384 - Attachment is obsolete: true
Attachment #750975 - Flags: review?(mrbkap)
(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.
Flags: needinfo?(vchang)
(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 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?
Attachment #750975 - Flags: review?(mrbkap)
> 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);
Flags: needinfo?(vchang)
> ::: 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.
(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.
Whiteboard: RN5/29
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #750975 - Attachment is obsolete: true
Attachment #755868 - Flags: review?(mrbkap)
Comment from Mozilla:HASH(0xabdffa4)
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.
Attachment #755868 - Flags: review?(mrbkap) → review+
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
Attachment #755868 - Attachment is obsolete: true
Push to birch directly because I have got the review+ in comment 18. 
 
https://hg.mozilla.org/projects/birch/rev/761e842067d8
Blocks: 854353
This patch also fixed the bug 854353 and should be leo+. Nominate it to leo ?
blocking-b2g: --- → leo?
Duplicate of this bug: 854353
I has tried this patch with b2g18 branch. We can land it to b2g17 once we get the leo+.
Hi Leo, can you try this patch ?
Flags: needinfo?(leo.bugzilla.gaia)
https://hg.mozilla.org/mozilla-central/rev/761e842067d8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
blocking-b2g: leo? → leo+
Depends on: 879927
Attachment #760780 - Attachment description: Patch v1.4 for b2g18 branch → Part1 - Patch v1.4 for b2g18 branch
Remove the trailing white space.
Attachment #760780 - Attachment is obsolete: true
Flags: needinfo?(vchang)
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.
Attachment #763326 - Flags: review?(mrbkap)
Flags: needinfo?(leo.bugzilla.gaia)
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.
Attachment #763402 - Flags: review?(mrbkap)
Attachment #763326 - Flags: review?(mrbkap) → review+
Attachment #763402 - Flags: review?(mrbkap) → review+
Can you please give the follow-up a commit message that isn't the same as the first part?
Modify the commit message for part 2 patch. Please help to commit this patch to inbound and m-c
Attachment #763402 - Attachment is obsolete: true
Modify the commit message for b2g18 part 2 patch.
Attachment #763326 - Attachment is obsolete: true
Attachment #763318 - Attachment description: Part1 - patch for b2g18 → b2g18 - Part1 - patch ready to commit for b2g18
Thanks :). I'll land this on b2g18 once it's merged over to m-c.
https://hg.mozilla.org/projects/birch/rev/2f49ab9a4da4
Duplicate of this bug: 884526
You need to log in before you can comment on or make changes to this bug.