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

RESOLVED FIXED in Firefox 24, Firefox OS v1.1hd

Status

Firefox OS
Gaia::Settings
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sync-1, Assigned: vchang)

Tracking

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(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)

Details

(Whiteboard: RN5/29)

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
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:
(Reporter)

Comment 1

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

Comment 2

5 years ago
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)

Updated

5 years ago
Flags: needinfo?(jcheng)
Vincent, any comments? thanks
Flags: needinfo?(vchang)
Flags: needinfo?(swu)
Flags: needinfo?(jcheng)

Comment 4

5 years ago
Yes, it's a known issue.

Assigned to Vincent.
Assignee: nobody → vchang
Flags: needinfo?(vchang)
(Assignee)

Comment 6

5 years ago
Created attachment 750384 [details] [diff] [review]
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.
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)
(Assignee)

Comment 8

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

Comment 9

5 years ago
Created attachment 750975 [details] [diff] [review]
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. :-)
Attachment #750384 - Attachment is obsolete: true
Attachment #750975 - Flags: review?(mrbkap)

Comment 10

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

Comment 13

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

Comment 14

5 years ago
> ::: 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.

Updated

5 years ago
Whiteboard: RN5/29
(Assignee)

Comment 16

5 years ago
Created attachment 755868 [details] [diff] [review]
Patch v1.3
Attachment #750975 - Attachment is obsolete: true
Attachment #755868 - Flags: review?(mrbkap)
(Reporter)

Comment 17

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

Comment 19

5 years ago
Created attachment 760762 [details] [diff] [review]
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
Attachment #755868 - Attachment is obsolete: true
(Assignee)

Comment 20

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

Updated

5 years ago
Blocks: 854353
(Assignee)

Comment 21

5 years ago
This patch also fixed the bug 854353 and should be leo+. Nominate it to leo ?
blocking-b2g: --- → leo?
(Assignee)

Updated

5 years ago
Duplicate of this bug: 854353
(Assignee)

Comment 23

5 years ago
Created attachment 760780 [details] [diff] [review]
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+.
(Assignee)

Comment 24

5 years ago
Hi Leo, can you try this patch ?
Flags: needinfo?(leo.bugzilla.gaia)
https://hg.mozilla.org/mozilla-central/rev/761e842067d8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
blocking-b2g: leo? → leo+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7fbf56d72d34
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
Target Milestone: --- → 1.1 QE3

Updated

5 years ago
Depends on: 879927
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
status-b2g18: fixed → affected
status-b2g-v1.1hd: fixed → affected
Flags: needinfo?(vchang)
Keywords: branch-patch-needed
(Assignee)

Updated

5 years ago
Attachment #760780 - Attachment description: Patch v1.4 for b2g18 branch → Part1 - Patch v1.4 for b2g18 branch
(Assignee)

Comment 29

5 years ago
Created attachment 763318 [details] [diff] [review]
b2g18 - Part1 - patch ready to commit for b2g18

Remove the trailing white space.
Attachment #760780 - Attachment is obsolete: true
Flags: needinfo?(vchang)
(Assignee)

Comment 30

5 years ago
Created attachment 763326 [details] [diff] [review]
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.
Attachment #763326 - Flags: review?(mrbkap)
Flags: needinfo?(leo.bugzilla.gaia)
(Assignee)

Comment 31

5 years ago
Created attachment 763402 [details] [diff] [review]
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.
Attachment #763402 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #763326 - Flags: review?(mrbkap) → review+

Updated

5 years ago
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?
(Assignee)

Comment 33

5 years ago
Created attachment 764510 [details] [diff] [review]
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
Attachment #763402 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
Created attachment 764513 [details] [diff] [review]
b2g18 - part 2 - Patch ready for commit to b2g18

Modify the commit message for b2g18 part 2 patch.
Attachment #763326 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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
You need to log in before you can comment on or make changes to this bug.