Last Comment Bug 758466 - B2G RIL: ensure radio state and 'ril.radio.disabled' setting value are known before modifying the radio state
: B2G RIL: ensure radio state and 'ril.radio.disabled' setting value are known ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Philipp von Weitershausen [:philikon]
:
:
Mentors:
Depends on:
Blocks: 744709
  Show dependency treegraph
 
Reported: 2012-05-24 17:53 PDT by Jonathan Griffin (:jgriffin)
Modified: 2012-05-26 05:17 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
logcat output (41.30 KB, text/plain)
2012-05-24 17:53 PDT, Jonathan Griffin (:jgriffin)
no flags Details
v1 (3.90 KB, patch)
2012-05-24 19:25 PDT, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Splinter Review
WIP v1 - Part II: Default value for 'ril.radio.disabled' setting. (981 bytes, patch)
2012-05-25 07:06 PDT, José Antonio Olivera Ortega [:jaoo]
philipp: feedback-
Details | Diff | Splinter Review

Description Jonathan Griffin (:jgriffin) 2012-05-24 17:53:02 PDT
Created attachment 627051 [details]
logcat output

All three Marionette dialer tests are failing in the same spot, while the receiver emulator is waiting for an incoming call.  

See http://ec2-107-20-108-245.compute-1.amazonaws.com/jenkins/job/webapi-marionette-test/8/console

I can reproduce the failures locally.

There is one unusual error in logcat:

E/GeckoConsole(   41): [JavaScript Error: "this._messages is null" {file: "resource://gre/modules/DOMRequestHelper.jsm" line: 88}]

Full logcat attached.

This is a recent regression; builds from a few days ago passed; but with latest m-c they fail.  Unfortunately, the CI wasn't running during this time so I can't say what changeset caused the regression.

The failing test locations:

http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_dial_answer.py#45
http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_dial_between_emulators.py#43
http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_dial_listeners.py#58

My B2G phone doesn't have a sim card, so I can't say whether calls from a B2G phone are actually getting made or not.
Comment 1 Philipp von Weitershausen [:philikon] 2012-05-24 18:12:51 PDT
Yup. This is a regression from bug 744709. We now no longer turn the radio on automatically but based on a setting. We'll have to modify these tests to turn the radio on first.
Comment 2 Philipp von Weitershausen [:philikon] 2012-05-24 18:20:21 PDT
(In reply to Jonathan Griffin (:jgriffin) from comment #0)
> E/GeckoConsole(   41): [JavaScript Error: "this._messages is null" {file:
> "resource://gre/modules/DOMRequestHelper.jsm" line: 88}]

I think this is unrelated. Potentially fall out from bug 757512.
Comment 3 Philipp von Weitershausen [:philikon] 2012-05-24 19:09:38 PDT
Alright, I *thought* I was on crack, because bug 744709 shouldn't have regressed us. Turns out, it's susceptible to a race condition that probably doesn't occur on phones but does occur on the emulator with its fake (and therefore probably much faster) RIL. We have two events occurring: asking the settings DB for the initial value of 'ril.radio.disabled', and waiting for the RIL to tell us what the current state is. We can only act and mutate the radio state when we know both! Patch coming up.
Comment 4 Philipp von Weitershausen [:philikon] 2012-05-24 19:25:55 PDT
Created attachment 627072 [details] [diff] [review]
v1
Comment 5 José Antonio Olivera Ortega [:jaoo] 2012-05-25 05:52:53 PDT
Comment on attachment 627072 [details] [diff] [review]
v1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +456,5 @@
> +  },
> +
> +  _ensureRadioState: function _ensureRadioState() {
> +    debug("Reported radio state is " + this.radioState.radioState +
> +          ", desired radio enabled state is " + this.radioEnabled);

s/this.radioEnabled/this._radioEnabled
Comment 6 José Antonio Olivera Ortega [:jaoo] 2012-05-25 07:06:30 PDT
Created attachment 627215 [details] [diff] [review]
WIP v1 - Part II: Default value for 'ril.radio.disabled' setting.

A way to set up a default value for 'ril.radio.disabled' setting. Ops! I have noticed right now this patch sets always 'ril.radio.disabled' to false. If the initial value is not 'null' is because the user set up a value and we have to keep it. Anyway that's the idea for setting up an initial value for 'ril.radio.disable' setting. I'll change a new WIP asap.
Comment 7 Philipp von Weitershausen [:philikon] 2012-05-25 07:54:27 PDT
Comment on attachment 627215 [details] [diff] [review]
WIP v1 - Part II: Default value for 'ril.radio.disabled' setting.

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

We set default prefs in the Gaia install part. Talk to gwagner, please. In any case, this should be a separate bug.
Comment 8 Philipp von Weitershausen [:philikon] 2012-05-25 07:55:06 PDT
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +456,5 @@
> > +  },
> > +
> > +  _ensureRadioState: function _ensureRadioState() {
> > +    debug("Reported radio state is " + this.radioState.radioState +
> > +          ", desired radio enabled state is " + this.radioEnabled);
> 
> s/this.radioEnabled/this._radioEnabled

Thanks for spotting this. I'll fix that before pushing.
Comment 9 José Antonio Olivera Ortega [:jaoo] 2012-05-25 08:08:30 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> Comment on attachment 627215 [details] [diff] [review]
> WIP v1 - Part II: Default value for 'ril.radio.disabled' setting.
> 
> Review of attachment 627215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We set default prefs in the Gaia install part. 

Ok, I did not know that. Thanks.
Comment 10 Philipp von Weitershausen [:philikon] 2012-05-25 11:08:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c000fd454e7a
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-05-26 05:17:53 PDT
https://hg.mozilla.org/mozilla-central/rev/c000fd454e7a

Note You need to log in before you can comment on or make changes to this bug.