887690 - B2G RIL: Support emergency callback mode

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=881863#c5

- handle unsolicited enter ECM from ril
- handle unsolicited exit ECM from ril
- send request exit ECM to ril
  * when user dial an non-emergency call
  * when timeout (already stay in ECM for a given period)
- send notification to DOM for mode change

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

reference log
http://pastebin.mozilla.org/2567218

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

CDMA EMERGENCY_CALLBACK_MODE related ril command is supported in latest reference-ril (b2g/master) but not in current revision set in repo (m/v1-train)

However I got some problem with latest ril version in emulator. It always show search network after power-on.

So for testing in marionette, I use the patch in Bug 887656.

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Add ecmchange event (idl)

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Add ecmchange event (dom)

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: Support emergency callback mode in ril

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4: Test emergency callback mode (xpcshell-test)

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5: Test emergency callback mode (marionette)

As I mentioned in Comment 2, the test need RIL's support.
It will not be added into the manifest now.

The purpose of this test is to simulate the whole scenario of entering emergency callback mode, which could only be triggered by make an emergency call on real network.

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Hi Hsinyi,

The API is not confirmed and still under discussion (Ref to Bug 881863).
This is just a WIP but workable patch. Please give me some opinions. Thank you.

Comment 9

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

Comment on attachment 768867 [details] [diff] [review]
Part 3: Support emergency callback mode in ril

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

I would like to hole the formal review until we decide how the API should look like.

::: dom/system/gonk/ril_worker.js
@@ +718,5 @@
>  
> +// Initialize buffers. This is a separate function so that unit tests can
> +// re-initialize the buffers at will.
> +// RIL uses Buf. Make sure we init Buf first.
> +Buf.init();

Why moving this? Any problems in jshint?

@@ +793,5 @@
> +    this._isInECM = libcutils.property_get("ril.cdma.inecmmode", "false") !== "false";
> +
> +    // Always ensure that we are not in emergency callback mode when init. The
> +    // code assumes that we have only one ril_worker in the system.
> +    if (this._isInECM) this._requestExitECM();

nit: don't forget {} even only one line in the body.

@@ +799,5 @@
> +    /**
> +     * Timeout value for emergency callback mode (default 5 mins). After the
> +     * period, we will request ril to exit the mode.
> +     */
> +    this._ecmTimeoutMs = parseInt(libcutils.property_get("ro.cdma.ecmexittimer",

Can't we simply have a constant in RIL? Any reason that you would like to have this property?

@@ +1712,5 @@
>      }
>  
> +    // Exit emergency callback mode when user dial a non-emergency call.
> +    if (this._isInECM) {
> +      this._requestExitECM();

Wouldn't modem/service provider take care of this?

@@ +3509,5 @@
> +  },
> +
> +  _handleChangedECM: function _handleChangedECM(inECM) {
> +    // Nothing changed.
> +    if (this._isInECM === inECM) return;

nit: coding style - remember {}.

@@ +5267,5 @@
>    this.ESN = result[2];
>    this.MEID = result[3];
>  };
> +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> +  if (options.rilRequestError) {

Curious: what happens if we request exiting emergency callback mode while we haven't been in the mode already? Request succeeds or fails?

@@ +5271,5 @@
> +  if (options.rilRequestError) {
> +    return;
> +  }
> +
> +  this._handleChangedECM(false);

Why do we need this here? Shouldn't the ECM value be updated until UNSOLICITED_EXIT_EMERGENCY_CALLBACK_MODE?

Comment 10

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> Comment on attachment 768867 [details] [diff] [review]
> Part 3: Support emergency callback mode in ril
> 
> Review of attachment 768867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would like to hole the formal review until we decide how the API should
> look like.
> 

Typo: I would like to *hold* the formal review ...  :)

Comment 11

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

Comment on attachment 768867 [details] [diff] [review]
Part 3: Support emergency callback mode in ril

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

::: dom/system/gonk/RILContentHelper.js
@@ +338,4 @@
>  function RILContentHelper() {
>    this.rilContext = {
>      cardState:            RIL.GECKO_CARDSTATE_UNKNOWN,
> +    ecm:                  false,

I would say I usually don't like abbreviation much. I don't see it really helps readability...

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 768867 [details] [diff] [review]
Part 3: Support emergency callback mode in ril

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

::: dom/system/gonk/RILContentHelper.js
@@ +338,4 @@
>  function RILContentHelper() {
>    this.rilContext = {
>      cardState:            RIL.GECKO_CARDSTATE_UNKNOWN,
> +    ecm:                  false,

I agree with you.
Any suggestion? If not, I will use 'emergencyCallbackMode' throughout all files.

::: dom/system/gonk/ril_worker.js
@@ +723,1 @@
>  

In RIL.initState(), I write
>> if (this._isInECM) this._requestExitECM();

The purpose is to force modem to exit the ECM mode when ril worker init.
The function _requestExitECM() need a ready Buf to send out the parcel.
So we should do Buf.init() before RIL.initState.

@@ +799,5 @@
> +    /**
> +     * Timeout value for emergency callback mode (default 5 mins). After the
> +     * period, we will request ril to exit the mode.
> +     */
> +    this._ecmTimeoutMs = parseInt(libcutils.property_get("ro.cdma.ecmexittimer",

I got some document mentioned that the timeout value is default to 5 mins but the actual value used is depended on the operator.

Now I set it as a ro property. (android do the same thing)
If the value is the same from phone to phone, simple constant is better.
However, I am not sure about this. We could confirm it later.

@@ +1713,5 @@
>  
> +    // Exit emergency callback mode when user dial a non-emergency call.
> +    if (this._isInECM) {
> +      this._requestExitECM();
> +    }

Not sure.
Try to do it ourself to mimic the behavior in the worst case.

Check this, search +WSOS (not spec, just one of the reference design)
http://www.multitech.com/en_US/DOCUMENTS/Collateral/manuals/S000478D.pdf

Modem has two mode: (1) timer reset, (2) basic.
In (1). modem take cares of this part, but the command to request exit explicitly is disabled. However, our RIL does expose the 'exit mode' request. So I guess the configuration of our modem might be something like case (2).

@@ +5267,5 @@
>    this.ESN = result[2];
>    this.MEID = result[3];
>  };
> +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> +  if (options.rilRequestError) {

Request fail: this is the behavior in emulator (android_modem)

@@ +5272,5 @@
> +    return;
> +  }
> +
> +  this._handleChangedECM(false);
> +};

Also, I am not sure of the real behavior.
No matter the cases, call this._handleChangedECM(false) twice is ok.
We only fire one mode change event to DOM.

Comment 13

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

(In reply to Szu-Yu Chen [:aknow] from comment #12)
> Comment on attachment 768867 [details] [diff] [review]
> Part 3: Support emergency callback mode in ril
> 
> Review of attachment 768867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +338,4 @@
> >  function RILContentHelper() {
> >    this.rilContext = {
> >      cardState:            RIL.GECKO_CARDSTATE_UNKNOWN,
> > +    ecm:                  false,
> 
> I agree with you.
> Any suggestion? If not, I will use 'emergencyCallbackMode' throughout all
> files.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +723,1 @@
> >  
> 
> In RIL.initState(), I write
> >> if (this._isInECM) this._requestExitECM();
> 
> The purpose is to force modem to exit the ECM mode when ril worker init.
> The function _requestExitECM() need a ready Buf to send out the parcel.
> So we should do Buf.init() before RIL.initState.
> 
> @@ +799,5 @@
> > +    /**
> > +     * Timeout value for emergency callback mode (default 5 mins). After the
> > +     * period, we will request ril to exit the mode.
> > +     */
> > +    this._ecmTimeoutMs = parseInt(libcutils.property_get("ro.cdma.ecmexittimer",
> 
> I got some document mentioned that the timeout value is default to 5 mins
> but the actual value used is depended on the operator.
> 
> Now I set it as a ro property. (android do the same thing)
> If the value is the same from phone to phone, simple constant is better.
> However, I am not sure about this. We could confirm it later.
> 
> @@ +1713,5 @@
> >  
> > +    // Exit emergency callback mode when user dial a non-emergency call.
> > +    if (this._isInECM) {
> > +      this._requestExitECM();
> > +    }
> 
> Not sure.
> Try to do it ourself to mimic the behavior in the worst case.
> 
> Check this, search +WSOS (not spec, just one of the reference design)
> http://www.multitech.com/en_US/DOCUMENTS/Collateral/manuals/S000478D.pdf
> 
> Modem has two mode: (1) timer reset, (2) basic.
> In (1). modem take cares of this part, but the command to request exit
> explicitly is disabled. However, our RIL does expose the 'exit mode'
> request. So I guess the configuration of our modem might be something like
> case (2).
> 
> @@ +5267,5 @@
> >    this.ESN = result[2];
> >    this.MEID = result[3];
> >  };
> > +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> > +  if (options.rilRequestError) {
> 
> Request fail: this is the behavior in emulator (android_modem)
> 

If that's the case, how are we going to make sure the property value is sync with modem?

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#2: Add emergency callback mode support (idl, webidl)

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Add emergency callback mode support (dom)

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#2: Add emergency callback mode support (ril)

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#2: Test emergency callback mode (xpcshell-test)

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#2: Test emergency callback mode (marionette)

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#3: Test emergency callback mode (xpcshell-test)

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#3: Add emergency callback mode support (idl, webidl)

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#3: Add emergency callback mode support (dom)

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3#3: Add emergency callback mode support (ril)

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#4: Test emergency callback mode (xpcshell-test)

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#3: Test emergency callback mode (marionette)

Comment 25

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 780908 [details] [diff] [review]
Part 5#3: Test emergency callback mode (marionette)

Hi edgar:

Please help for "switch technology" part. Thx.

Comment 26

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

Comment on attachment 780905 [details] [diff] [review]
Part 2#3: Add emergency callback mode support (dom)

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

Leave this part to DOM peer ;)

Comment 27

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

Comment on attachment 780906 [details] [diff] [review]
Part 3#3: Add emergency callback mode support (ril)

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

::: dom/system/gonk/ril_worker.js
@@ +3724,5 @@
> +    }
> +  },
> +
> +  _handleChangedEmergencyCbMode: function _handleChangedEmergencyCbMode(active) {
> +    // Nothing changed.

nit: How about removing the comment as the code is very straightforward?

@@ +3735,5 @@
> +      let ril = this;
> +      this._cancelEmergencyCbModeTimeout();
> +      this._exitEmergencyCbModeTimeoutID = setTimeout(function() {
> +          ril.exitEmergencyCbMode(); },
> +          EMERGENCY_CB_MODE_TIMEOUT_MS);

nit: coding style

this._exitEmergencyCbModeTimeoutID = setTimeout(function() {
   ril.exitEmergencyCbMode();
}, EMERGENCY_CB_MODE_TIMEOUT_MS);

@@ +5651,5 @@
>    this.ESN = result[2];
>    this.MEID = result[3];
>  };
> +RIL[REQUEST_EXIT_EMERGENCY_CALLBACK_MODE] = function REQUEST_EXIT_EMERGENCY_CALLBACK_MODE(length, options) {
> +  debug("[aknow]: request exit ecm " + JSON.stringify(options));

Please ~ ;)

Comment 28

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

Comment on attachment 780908 [details] [diff] [review]
Part 5#3: Test emergency callback mode (marionette)

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

Thank you for the marionette tests which look good. However, I was told that we still have some problems in emulator technology switch that some network states might not be correctly updated. That problem isn't obvious in the separate telephony tests, but I am concerned some unexpected behaviours occurs when we do a bunch of tests in sequence. So, I would suggest move the marionette tests to a follow-up bug. Let's land them later.

Comment 29

[Edgar Chen [:edgar]]

Comment on attachment 780908 [details] [diff] [review]
Part 5#3: Test emergency callback mode (marionette)

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

Agree with Hsinyi's comment :)

Comment 30

[Mounir Lamouri (:mounir)]

Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Hsin-Yi, can you do that sr?

Comment 31

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

(In reply to Mounir Lamouri (:mounir) from comment #30)
> Comment on attachment 780903 [details] [diff] [review]
> Part 1#3: Add emergency callback mode support (idl, webidl)
> 
> Review of attachment 780903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hsin-Yi, can you do that sr?

Sure!

Comment 32

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

Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Looks good to me.

Comment 33

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

Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Mounir, the api design looks good to me. It supports what the user story needs. 

However, per my understanding, we want this MozEmergencyCbModeEvent being created and fired by MobileConnection, not created by |new MozEmergencyCbModeEvent()| on web pages. So I thought it's good no [constructor] here, but I was just told that all event interfaces are supposed to have [constructor]. I am delivering the sr? back to you since I am not sure the part of event webidl, and would like to have your comment on it. Thank you!

Comment 34

[Mounir Lamouri (:mounir)]

Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

Yes, events should have a constructor, feel free to ping smaug for more information.

Comment 35

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

Comment on attachment 780903 [details] [diff] [review]
Part 1#3: Add emergency callback mode support (idl, webidl)

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

A lot of news things to learn about webidl. I am sorry that we need to have [constructor] for all events. r- for that.
And we would need another DOM/webidl peer for the webidl review. bent or smaug could be nice candidates. :)

Comment 36

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#4: Add emergency callback mode support (idl, webidl)

Comment 37

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#4: Add emergency callback mode support (dom)

Comment 38

[Mounir Lamouri (:mounir)]

Comment on attachment 784902 [details] [diff] [review]
Part 1#4: Add emergency callback mode support (idl, webidl)

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

Bouncing to smaug, to make sure the event is properly defined.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 784902 [details] [diff] [review]
Part 1#4: Add emergency callback mode support (idl, webidl)

Could you please use event codegen for the event.
I know it requires .idl still, but at least we wouldn't have .h and .cpp for the
event.

Comment 40

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #39)
> Comment on attachment 784902 [details] [diff] [review]
> Part 1#4: Add emergency callback mode support (idl, webidl)
> 
> Could you please use event codegen for the event.
> I know it requires .idl still, but at least we wouldn't have .h and .cpp for
> the
> event.

What's the policy of idl? I thought we were going to replace all the idl by webidl in the future.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

No. We're not going to remove all .idl. We're trying to remove most of the .idl which
define interfaces for web content. And yes, that means also these events, but for
now events should use event codegenerator whenever possible. That makes code reviewing easier, 
mistakes harder and more importantly, changes to all the event implementations a lot easier.

(I'm hoping to have a code generator for .webidl only events early this autumn.)

Comment 42

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#5: Add emergency callback mode support (idl, webidl)

Comment 43

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#5: Add emergency callback mode support (dom)

Comment 44

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 786142 [details] [diff] [review]
Part 2#5: Add emergency callback mode support (dom)

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

::: dom/network/src/MobileConnection.cpp
@@ +549,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDOMEvent> event;
> +  NS_NewDOMMozEmergencyCbModeEvent(getter_AddRefs(event), this, nullptr,
> +                                   nullptr);

Please MOZ_ASSERT(event) here.

Comment 45

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=4b700a7d1975

Comment 46

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1: Add emergency callback mode support (idl, webidl). r=hsinyi. sr=bugs

Comment 47

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 2: Add emergency callback mode support (dom). r=bent

Comment 48

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 3: Add emergency callback mode support (ril). r=hsinyi

Comment 49

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 4: Test emergency callback mode (xpcshell-test). r=hsinyi

Comment 50

[Enpei]

Hi Aknow,

Can it enter ECM even without UIM card?

Comment 51

[Szu-Yu Chen [:aknow] (inactive)]

Actually, entering ECM is controlled by modem. Gecko just pass the event from modem to gaia, so we could not control the behavior.

However, I think that it should got the entering event from modem if you dial an emergency call.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1defeb691f5c
https://hg.mozilla.org/integration/b2g-inbound/rev/9eea090ed6f7
https://hg.mozilla.org/integration/b2g-inbound/rev/e2c72cb029f4
https://hg.mozilla.org/integration/b2g-inbound/rev/106ae6eee902

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing pretty frequent B2G mochitest-2 failures.
https://hg.mozilla.org/integration/b2g-inbound/rev/c146d402a55f

https://tbpl.mozilla.org/php/getParsedLog.php?id=26446516&tree=B2g-Inbound

12:41:01     INFO -  pushing /system/bin/busybox
12:41:01     INFO -  waiting for system-message-listener-ready...
12:41:01     INFO -  done
12:41:01     INFO -  args: [u'/builds/slave/test/build/xre/bin/xpcshell', '-g', '/builds/slave/test/build/xre/bin', '-v', '170', '-f', '/builds/slave/test/build/xre/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpUaMZZW';const _SERVER_PORT = '8888'; const _SERVER_ADDR = '10.0.2.2';\n                     const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', './server.js']
12:41:01     INFO -  1376336428770	Marionette	INFO	MarionetteComponent loaded
12:41:01     INFO -  1376336429059	Marionette	INFO	marionette enabled via build flag and pref
12:41:01     INFO -  -*- RadioInterfaceLayer: 1 interfaces
12:41:01     INFO -  1376336435753	Marionette	INFO	marionette-server.js loaded
12:41:01     INFO -  1376336437073	Marionette	INFO	B2G emulator: yes
12:41:01     INFO -  1376336437085	Marionette	INFO	Device detected is generic
12:41:01     INFO -  1376336437095	Marionette	INFO	Bypassing offline status.
12:41:01     INFO -  1376336437113	Marionette	INFO	Listening on port 2828
12:41:01     INFO -  1376336437130	Marionette	INFO	Marionette server ready
12:41:01     INFO -  1376336438657	Marionette	DEBUG	accepted connection on 127.0.0.1:38559
12:41:01     INFO -  PermissionsTable.jsm: expandPermissions: Unknown Permission: network-tcpPermissionsTable.jsm: expandPermissions: Unknown Permission: network-httpPermissionsInstaller.jsm: 'network-tcp' is not a valid Webapps permission name.PermissionsInstaller.jsm: 'network-http' is not a valid Webapps permission name.1376336455139	Marionette	DEBUG	accepted connection on 127.0.0.1:38560
12:41:01     INFO -  1376336455352	Marionette	TRACE	Got: {"to": "root", "type": "getMarionetteID"}
12:41:01     INFO -  1376336456512	Marionette	TRACE	Got: {"to": "0", "type": "newSession"}
12:41:01     INFO -  System JS : ERROR jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1148
12:41:01     INFO -                       Error: Don't know about this message type: undefined
12:41:01     INFO -  System JS : ERROR chrome://satchel/content/formSubmitListener.js:15
12:41:01     INFO -                       ReferenceError: XPCOMUtils is not defined

You can ignore all the warnings about forms.js - that's a different issue being tracked.

Comment 54

[Szu-Yu Chen [:aknow] (inactive)]

22:57:02     INFO -  System JS : ERROR jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1118
22:57:02     INFO -                       Error: Don't know about this message type: undefined

This is not the reason that cause test fail. We could also found this line in other pass log.

12:41:01     INFO -  System JS : ERROR chrome://satchel/content/formSubmitListener.js:15
12:41:01     INFO -                       ReferenceError: XPCOMUtils is not defined

Also appears in Bug 904224
https://tbpl.mozilla.org/php/getParsedLog.php?id=26445003&tree=B2g-Inbound

Comment 55

[Szu-Yu Chen [:aknow] (inactive)]

Hi Ryan,

Test on try without any modification.
mochitest-2 seems good
https://tbpl.mozilla.org/?tree=Try&rev=483793a4689e

Comment 56

[Ryan VanderMeulen [:RyanVM]]

(In reply to Szu-Yu Chen [:aknow] from comment #54)
> Also appears in Bug 904224
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26445003&tree=B2g-Inbound

Because I used the same log link. There were also many instances of bug 904224 that occurred in non-failing runs and didn't have that error.

I've retriggered mochitest-2 on your Try push (it was an intermittent failure, one run won't really tell us if it was a spurious issue or not).

Comment 57

[Ryan VanderMeulen [:RyanVM]]

But I actually do see that error elsewhere, so maybe it was something spurious going on yesterday. The retriggers on Try look good, so I will re-land. Sorry for the churn.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Except part 3 doesn't apply anymore. Please rebase :(

Comment 59

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 1#2: Add emergency callback mode support (idl, webidl). r=hsinyi. sr=bugs

Comment 60

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 2#2: Add emergency callback mode support (dom). r=bent

Comment 61

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 3#2: Add emergency callback mode support (ril). r=hsinyi

Comment 62

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Part 4#2: Test emergency callback mode (xpcshell-test). r=hsinyi

Comment 63

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=d9328a4e27d2

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/25fe4e45ddb8
https://hg.mozilla.org/integration/b2g-inbound/rev/4d6ed21a071a
https://hg.mozilla.org/integration/b2g-inbound/rev/ca56d6ec8474
https://hg.mozilla.org/integration/b2g-inbound/rev/eb92e6079841

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25fe4e45ddb8
https://hg.mozilla.org/mozilla-central/rev/4d6ed21a071a
https://hg.mozilla.org/mozilla-central/rev/ca56d6ec8474
https://hg.mozilla.org/mozilla-central/rev/eb92e6079841

Comment 66

[Pooja]

HI Chen,
In gaia at [1] we already uses timer to track ECM mode timeout value (5min),which we displays ti UI, then why does the ril_worker.js at [2] needs to maintain a separate timer in Emergency callback mode.

Can't gaia send the Exit Emergency callback mode request to RIL whenever timer expires?

This query is because sometime possibly the UI can go out of sync if for some reason RIL didn't exit the callback mode but UI removed that information.

[1] https://github.com/steveck-chung/gaia/commit/5e4f51cdaf4581301b1834fc584d78e5ea6e1604
[2] https://hg.mozilla.org/mozilla-central/rev/ca56d6ec8474#l3.124

Thanks,
Pooja

Comment 67

[Szu-Yu Chen [:aknow] (inactive)]

> Can't gaia send the Exit Emergency callback mode request to RIL whenever
> timer expires?
> 
> This query is because sometime possibly the UI can go out of sync if for
> some reason RIL didn't exit the callback mode but UI removed that
> information.

It is a protection in case gaia doesn't send the exit request. Thus, even without the gaia (correctly handle the related event), all the functionality works well. We don't have to worry about being lock in emergency callback mode and cannot get out of it.

It should be OK if gecko exit the mode first and then gaia send the exit request when timeout. If it cause the problem, we should fix it. My idea is that no matter gaia send the request or not, everything should be fine.

Exit request might fail. UI should not directly remove the information when timeout occurs or just after it send out the exit request. The better way is to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia will receive that event. Based on the event, we could know the entering/leaving time and then show the correct info.

Did you find any problem or risk in current implementation?

Comment 68

[Anshul]

(In reply to Szu-Yu Chen [:aknow] from comment #67)
> > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > timer expires?
> > 
> > This query is because sometime possibly the UI can go out of sync if for
> > some reason RIL didn't exit the callback mode but UI removed that
> > information.
> 
> It is a protection in case gaia doesn't send the exit request. Thus, even
> without the gaia (correctly handle the related event), all the functionality
> works well. We don't have to worry about being lock in emergency callback
> mode and cannot get out of it.
> 
> It should be OK if gecko exit the mode first and then gaia send the exit
> request when timeout. If it cause the problem, we should fix it. My idea is
> that no matter gaia send the request or not, everything should be fine.
> 
> Exit request might fail. UI should not directly remove the information when
> timeout occurs or just after it send out the exit request. The better way is
> to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> will receive that event. Based on the event, we could know the
> entering/leaving time and then show the correct info.
Szu-Yu, I now understand what you are going for. However there is a potential for race condition between gaia and gecko. Since gecko already manages the timer and report the emergencycbmodechange event the timer in gaia seems unnecessary. 

> 
> Did you find any problem or risk in current implementation?

Comment 69

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Anshul from comment #68)
> (In reply to Szu-Yu Chen [:aknow] from comment #67)
> > > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > > timer expires?
> > > 
> > > This query is because sometime possibly the UI can go out of sync if for
> > > some reason RIL didn't exit the callback mode but UI removed that
> > > information.
> > 
> > It is a protection in case gaia doesn't send the exit request. Thus, even
> > without the gaia (correctly handle the related event), all the functionality
> > works well. We don't have to worry about being lock in emergency callback
> > mode and cannot get out of it.
> > 
> > It should be OK if gecko exit the mode first and then gaia send the exit
> > request when timeout. If it cause the problem, we should fix it. My idea is
> > that no matter gaia send the request or not, everything should be fine.
> > 
> > Exit request might fail. UI should not directly remove the information when
> > timeout occurs or just after it send out the exit request. The better way is
> > to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> > will receive that event. Based on the event, we could know the
> > entering/leaving time and then show the correct info.
> Szu-Yu, I now understand what you are going for. However there is a
> potential for race condition between gaia and gecko. Since gecko already
> manages the timer and report the emergencycbmodechange event the timer in
> gaia seems unnecessary. 

Gaia side needs a timer to show the countdown value on UI. So I think the timer could not be removed.

What's the problem for this race condition? I have consider following two cases. Both of them looks good to me.
1. If gaia gets timeout first and then sends the exit request, gecko will stop its timer when handling the request.
2. If gecko gets timeout first and send the exit request by itself, gaia should receive the mode change event during its countdown. Whenever gaia receives a leaving event, it should clear the related UI. The same situation occurs when user dialing a non-emergency call during the emergency callback mode.

Comment 70

[Anshul]

(In reply to Szu-Yu Chen [:aknow] from comment #69)
> (In reply to Anshul from comment #68)
> > (In reply to Szu-Yu Chen [:aknow] from comment #67)
> > > > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > > > timer expires?
> > > > 
> > > > This query is because sometime possibly the UI can go out of sync if for
> > > > some reason RIL didn't exit the callback mode but UI removed that
> > > > information.
> > > 
> > > It is a protection in case gaia doesn't send the exit request. Thus, even
> > > without the gaia (correctly handle the related event), all the functionality
> > > works well. We don't have to worry about being lock in emergency callback
> > > mode and cannot get out of it.
> > > 
> > > It should be OK if gecko exit the mode first and then gaia send the exit
> > > request when timeout. If it cause the problem, we should fix it. My idea is
> > > that no matter gaia send the request or not, everything should be fine.
> > > 
> > > Exit request might fail. UI should not directly remove the information when
> > > timeout occurs or just after it send out the exit request. The better way is
> > > to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> > > will receive that event. Based on the event, we could know the
> > > entering/leaving time and then show the correct info.
> > Szu-Yu, I now understand what you are going for. However there is a
> > potential for race condition between gaia and gecko. Since gecko already
> > manages the timer and report the emergencycbmodechange event the timer in
> > gaia seems unnecessary. 
> 
> Gaia side needs a timer to show the countdown value on UI. So I think the
> timer could not be removed.
> 
> What's the problem for this race condition? I have consider following two
> cases. Both of them looks good to me.
> 1. If gaia gets timeout first and then sends the exit request, gecko will
> stop its timer when handling the request.
> 2. If gecko gets timeout first and send the exit request by itself, gaia
> should receive the mode change event during its countdown. Whenever gaia
> receives a leaving event, it should clear the related UI. The same situation
> occurs when user dialing a non-emergency call during the emergency callback
> mode.
When gecko timesout and send the request to RIL to exit the emergency callback mode gecko doesn't notify gaia that emergency mode is exited http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#1323. Also sine gecko originated the request explictly most likely RIL won't respond with unsol emergency callback mode and therefore no callback mode change event would be sent to gaia.

Comment 71

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Anshul from comment #70)
> (In reply to Szu-Yu Chen [:aknow] from comment #69)
> > (In reply to Anshul from comment #68)
> > > (In reply to Szu-Yu Chen [:aknow] from comment #67)
> > > > > Can't gaia send the Exit Emergency callback mode request to RIL whenever
> > > > > timer expires?
> > > > > 
> > > > > This query is because sometime possibly the UI can go out of sync if for
> > > > > some reason RIL didn't exit the callback mode but UI removed that
> > > > > information.
> > > > 
> > > > It is a protection in case gaia doesn't send the exit request. Thus, even
> > > > without the gaia (correctly handle the related event), all the functionality
> > > > works well. We don't have to worry about being lock in emergency callback
> > > > mode and cannot get out of it.
> > > > 
> > > > It should be OK if gecko exit the mode first and then gaia send the exit
> > > > request when timeout. If it cause the problem, we should fix it. My idea is
> > > > that no matter gaia send the request or not, everything should be fine.
> > > > 
> > > > Exit request might fail. UI should not directly remove the information when
> > > > timeout occurs or just after it send out the exit request. The better way is
> > > > to check 'emergencycbmodechange' event. Whenever RIL changes the mode, gaia
> > > > will receive that event. Based on the event, we could know the
> > > > entering/leaving time and then show the correct info.
> > > Szu-Yu, I now understand what you are going for. However there is a
> > > potential for race condition between gaia and gecko. Since gecko already
> > > manages the timer and report the emergencycbmodechange event the timer in
> > > gaia seems unnecessary. 
> > 
> > Gaia side needs a timer to show the countdown value on UI. So I think the
> > timer could not be removed.
> > 
> > What's the problem for this race condition? I have consider following two
> > cases. Both of them looks good to me.
> > 1. If gaia gets timeout first and then sends the exit request, gecko will
> > stop its timer when handling the request.
> > 2. If gecko gets timeout first and send the exit request by itself, gaia
> > should receive the mode change event during its countdown. Whenever gaia
> > receives a leaving event, it should clear the related UI. The same situation
> > occurs when user dialing a non-emergency call during the emergency callback
> > mode.
> When gecko timesout and send the request to RIL to exit the emergency
> callback mode gecko doesn't notify gaia that emergency mode is exited
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#1323. Also sine gecko originated the request explictly most likely RIL
> won't respond with unsol emergency callback mode and therefore no callback
> mode change event would be sent to gaia.

Anshul,

Did you mean when we send out the exit request in the emergency callback mode, there will not be an unsolicited response of "callback mode change" from ril. We should determine whether the mode changed by success/error result of the exit request.

If so, this is a flaw in current design. We should fix it. Really thank you for pointing out the problem.

Comment 72

[Anshul]

Szu-Yu Chen, I have to test this to confirm but I would suppose RIL won't send an unsol as gecko explicitly requested for it. If my hunch is true (and I will confirm that) then gaia sending an exit emergency mode after timer expires as per my original suggestion seems like a solution.

Comment 73

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Anshul from comment #72)
> Szu-Yu Chen, I have to test this to confirm but I would suppose RIL won't
> send an unsol as gecko explicitly requested for it. If my hunch is true (and
> I will confirm that) then gaia sending an exit emergency mode after timer
> expires as per my original suggestion seems like a solution.

Yes, it could also be achieved by gecko. Gecko could notify gaia that emergency mode is exited after its exiting request is successful.

Another question is dialing a non-emergency call during the emergency callback mode. Currently, gecko also explicitly request the mode exit when it found the case happened. We need a way to notify gaia about the mode change if ril doesn't send the unsol response. Using above solution could also resolve this problem. Otherwise, we should move all the logic to gaia side.