965143 - [B2G][RIL] JavaScript error when connecting to a network manually and then set automatically selection

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Arthur Chen [:arthurcc][inactive after 6/30]]

* Build Number  
Inari
Gaia      59e477229162926981cc00cf5d6c53f9614f17dd
Gecko     http://hg.mozilla.org/mozilla-central/rev/7e79536aca0a
BuildID   20140128161516
Version   29.0a1

* Steps
1. Go to Settings > Cellular & Data > Network operator
2. Disable automatic selection
3. Connecting to any network
4. Enable automatic selection

* Expected result
No JavaScript error

* Actual Result
E/GeckoConsole(  523): [JavaScript Error: "[Exception... "Error: Already selecting a network: [xpconnect wrapped nsIDOMMozMobileNetworkInfo]'Error: Already selecting a network: [xpconnect wrapped nsIDOMMozMobileNetworkInfo]' when calling method: [nsIMobileConnectionProvider::selectNetworkAutomatically]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame :: app://settings.gaiamobile.org/js/carrier.js :: cs_initOperatorSelector/opAutoSelectInput.onchange :: line 368"  data: no]" {file: "app://settings.gaiamobile.org/js/carrier.js" line: 368}]
E/GeckoConsole(  523): [JavaScript Error: "NS_ERROR_XPC_JS_THREW_JS_OBJECT: Error: Already selecting a network: [xpconnect wrapped nsIDOMMozMobileNetworkInfo]'Error: Already selecting a network: [xpconnect wrapped nsIDOMMozMobileNetworkInfo]' when calling method: [nsIMobileConnectionProvider::selectNetworkAutomatically]" {file: "app://settings.gaiamobile.org/js/carrier.js" line: 368}]

Comment 1

[Edgar Chen [:edgar]]

Gecko doesn't allow user switch to automatic selection if the previous selection request is still under processing. In such case, you will get this JavaScript error, 

E/GeckoConsole(  523): [JavaScript Error: "NS_ERROR_XPC_JS_THREW_JS_OBJECT: Error: Already selecting a network: [xpconnect wrapped nsIDOMMozMobileNetworkInfo]'Error: Already selecting a network: [xpconnect wrapped nsIDOMMozMobileNetworkInfo]' when calling method: [nsIMobileConnectionProvider::selectNetworkAutomatically]" {file: "app://settings.gaiamobile.org/js/carrier.js" line: 368}]

Thank you.

Comment 2

[Edgar Chen [:edgar]]

This is an expected behavior, so close as WONTFIX.

Comment 3

[Edgar Chen [:edgar]]

After offline discussion, since electNetwork()/selectNetworkAutomatically() returns a DOMRequest, it seems more reasonable to report error by dispatching an error event instead of thrown an exception. So reopen this bug to address it.

Comment 4

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8437544 [details] [diff] [review]
Patch, v1

Hi Hsinyi, Could you help to review this? Thank you

Comment 6

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

Comment on attachment 8437544 [details] [diff] [review]
Patch, v1

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

I like the new behaviour.

::: dom/mobileconnection/tests/marionette/test_mobile_networks.js
@@ +100,5 @@
>  }
>  
>  function testSelectNetworkErrors(aNetworkToSelect, aAnotherNetwork) {
>    throwsException(() => mobileConnection.selectNetwork(null));
>    throwsException(() => mobileConnection.selectNetwork({}));

These two should be revised accordingly, right?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2449,5 @@
> +  /**
> +   * The networks that are currently trying to be selected (or "automatic").
> +   * This helps ensure that only one network per client is selected at a time.
> +   */
> +  _selectingNetworks: null,

nit: s/_selectingNetworks/_selectingNetwork/

::: dom/system/gonk/ril_worker.js
@@ +6156,5 @@
>  RilObject.prototype[REQUEST_SET_NETWORK_SELECTION_AUTOMATIC] = function REQUEST_SET_NETWORK_SELECTION_AUTOMATIC(length, options) {
>    if (options.rilRequestError) {
>      options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +  } else {
> +    this._updateNetworkSelectionMode(GECKO_NETWORK_SELECTION_AUTOMATIC);

Nice!

Comment 7

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> Comment on attachment 8437544 [details] [diff] [review]
> Patch, v1
> 
> Review of attachment 8437544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the new behaviour.
> 
> ::: dom/mobileconnection/tests/marionette/test_mobile_networks.js
> @@ +100,5 @@
> >  }
> >  
> >  function testSelectNetworkErrors(aNetworkToSelect, aAnotherNetwork) {
> >    throwsException(() => mobileConnection.selectNetwork(null));
> >    throwsException(() => mobileConnection.selectNetwork({}));
> 
> These two should be revised accordingly, right?
No, we don't need to revise them.
Above two exceptions are both thrown by MozMobileConnectionBinding (not RILContentHelper), because we did not pass a MozMobileNetwork wrapped object as input argument.

Comment 8

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

(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> > Comment on attachment 8437544 [details] [diff] [review]
> > Patch, v1
> > 
> > Review of attachment 8437544 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I like the new behaviour.
> > 
> > ::: dom/mobileconnection/tests/marionette/test_mobile_networks.js
> > @@ +100,5 @@
> > >  }
> > >  
> > >  function testSelectNetworkErrors(aNetworkToSelect, aAnotherNetwork) {
> > >    throwsException(() => mobileConnection.selectNetwork(null));
> > >    throwsException(() => mobileConnection.selectNetwork({}));
> > 
> > These two should be revised accordingly, right?
> No, we don't need to revise them.
> Above two exceptions are both thrown by MozMobileConnectionBinding (not
> RILContentHelper), because we did not pass a MozMobileNetwork wrapped object
> as input argument.

Got it! Thanks~

Comment 9

[Edgar Chen [:edgar]]

Attachment: Patch, v2, r=hsinyi

Address nit in comment #6:
- s/_selectingNetworks/_selectingNetwork/

Comment 10

[Edgar Chen [:edgar]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=1f67e73de25a

Comment 11

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f3ba91f22390

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f3ba91f22390