Dialer, messaging and settings app not working

RESOLVED FIXED in Firefox 40

Status

defect
P1
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: anshulj, Assigned: edgar)

Tracking

unspecified
2.2 S9 (3apr)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [caf priority: p1][CR 807538])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Dialer throws the following exception when attempting to dial a call
Communications( 3557): [JavaScript Error: "ReferenceError: MmiManager is not defined" {file: "app://communications.gaiamobile.org/dialer/gaia_build_defer_index.js" line: 151}]

Similar error are coming from settings app when I try to open say a Call Settings.
Settings( 3702): [JavaScript Error: "TypeError: s[0] is undefined" {file: "app://settings.gaiamobile.org/js/telephony_items_handler.js" line: 1}]
(Reporter)

Comment 1

4 years ago
I have narrowed the issue down to bug 1091307 that is causing the issue. We have MOZ RIL disabled in our builds and suspect that to be the reason for these apps to be not working. Since we have an alternate telephony implementation running, these app should technically work.

To reproduce the issue add ac_add_options --disable-mozril-geoloc to your mozconfig (as per bug 1091307#c12), make a build and flash the device. There is one issue however, which is that phone doesn't boot up without an alternate telephony implementation in the absence of Moz RIL and location. Not sure if that is a quick fix on Moz's side, if not, I would be more than happy to test the provided patches.
Summary: Dialer. messaging and settings app not working → Dialer, messaging and settings app not working
Whiteboard: [CR 807538]
Whiteboard: [CR 807538] → [caf priority: p2][CR 807538]
(2.2+, as this needs to be fixed to commercialize v2.2)
Severity: normal → critical
blocking-b2g: 2.2? → 2.2+
Depends on: 1091307
Whiteboard: [caf priority: p2][CR 807538] → [caf priority: p1][CR 807538]

Comment 3

4 years ago
Hi! Hsin-Yi,

Could your team help to take a look? Thanks

--
Keven
Flags: needinfo?(htsai)

Comment 4

4 years ago
Hi Fabrice,
 It looks like bug 1091307 introduce this regression.
Could you please provide some suggestion here?

Thanks!!
Flags: needinfo?(htsai) → needinfo?(fabrice)
I'll let Hsin-Yi handle that.
Flags: needinfo?(fabrice)
Hi Edgar,
Could you please help this first and work closely with Anshul, though per the talk with Anshul we believed that it's hardly able to reproduce this issue locally on moz side because we didn't have caf ril?
Flags: needinfo?(echen)
(Assignee)

Comment 7

4 years ago
Hi Anshul, we are unable to reproduce this locally, I would like to clarify something regarding to alternate telephony implementation first.

Take telephonyService as example,
1. AFAIK, alternate telephony implementation doesn't touch any IPC code, e.g. IPDL, for the telephony service lives in content process it reuses the Moz implementation, e.g. TelephonyIPCService [1]. Does my understanding correct? (If yes, I guess we should still register the components in nsLayoutModule.cpp)
2. What contract id does alternate telephony implementation use?
- @mozilla.org/telephony/telephonyservice;1
- @mozilla.org/telephony/gonktelephonyservice;1
The reason I asked this is because service creator [2] will check the process type to create corresponding service: If the process type is GeckoProcessType_Content, returns a TelephonyIPCService. Otherwise, returns a "@mozilla.org/telephony/gonktelephonyservice;1" service (and this should be the alternate telephony implementation).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/ipc/TelephonyIPCService.cpp
[2] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#737-751
Flags: needinfo?(anshulj)
(Reporter)

Comment 8

4 years ago
(In reply to Edgar Chen [:edgar][:echen] (OOO 3/21~4/6) from comment #7)
> Hi Anshul, we are unable to reproduce this locally, I would like to clarify
> something regarding to alternate telephony implementation first.
> 
> Take telephonyService as example,
> 1. AFAIK, alternate telephony implementation doesn't touch any IPC code,
> e.g. IPDL, for the telephony service lives in content process it reuses the
> Moz implementation, e.g. TelephonyIPCService [1]. Does my understanding
> correct? (If yes, I guess we should still register the components in
> nsLayoutModule.cpp)
Yes we do not touch and IPC or DOM code as use it as is.

> 2. What contract id does alternate telephony implementation use?
> - @mozilla.org/telephony/telephonyservice;1
> - @mozilla.org/telephony/gonktelephonyservice;1
> The reason I asked this is because service creator [2] will check the
> process type to create corresponding service: If the process type is
> GeckoProcessType_Content, returns a TelephonyIPCService. Otherwise, returns
> a "@mozilla.org/telephony/gonktelephonyservice;1" service (and this should
> be the alternate telephony implementation).
We use "@mozilla.org/telephony/telephonyservice;1" to provide an alternate implementation of telephony service.

> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/telephony/ipc/
> TelephonyIPCService.cpp
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.
> cpp#737-751
Flags: needinfo?(anshulj)
(Reporter)

Comment 9

4 years ago
We don't use nsIGonkTelephonyService as we don't implement nsIGonkTelephonyService interface nor use it. We see it as an internal Moz RIL implementation on how RadioInterafaceLayer.js/ril_worker.js talk to Mozilla's telephony service implementation.
(Assignee)

Comment 10

4 years ago
(In reply to Anshul from comment #9)
> We don't use nsIGonkTelephonyService as we don't implement
> nsIGonkTelephonyService interface nor use it. We see it as an internal Moz
> RIL implementation on how RadioInterafaceLayer.js/ril_worker.js talk to
> Mozilla's telephony service implementation.

Yes, you don't have to implement nsIGonkTelephonyService, nsIGonkTelephonyService is a internal interface and should be only used for mozilla's implementation.

But I guess alternate implementation of telephony service (which implements only nsITelephonyService is enough) still need to register as "@mozilla.org/telephony/gonktelephonyservice;1", otherwise service creator [1] may not be able to return a correct service instance.

I will provide a test patch to enable service creator in nsLayoutModule.cpp back, and please kindly help to try the patch on your side to see if it helps.

Thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#737-751
(Assignee)

Comment 11

4 years ago
Posted patch Test Patch, v1 (obsolete) — Splinter Review
(Assignee)

Comment 12

4 years ago
Comment on attachment 8578445 [details] [diff] [review]
Test Patch, v1

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

Hi Anshul, could you help to try this patch?
1. Apply this patch.
2. Use "@mozilla.org/telephony/gonktelephonyservice;1" for telephony service (same changes for other telephony components).
Attachment #8578445 - Flags: feedback?(anshulj)
(Assignee)

Updated

4 years ago
Flags: needinfo?(echen)

Updated

4 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(echen)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8578445 [details] [diff] [review]
Test Patch, v1

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

Edgar, this patch alone, without the need of registering our component as gonktelephonyservice seems to have fixed the issue. Seems like you have removed featurization (DISABLE_MOZ_RIL_GEOLOC) of telephony related code in file nsLayoutModule.cpp but not for location. Why is that; wouldn't location have the same issue?

Updated

4 years ago
Assignee: nobody → echen
(Assignee)

Comment 14

4 years ago
(In reply to Anshul from comment #13)
> Comment on attachment 8578445 [details] [diff] [review]
> Test Patch, v1
> 
> Review of attachment 8578445 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Edgar, this patch alone, without the need of registering our component as
> gonktelephonyservice seems to have fixed the issue. Seems like you have
> removed featurization (DISABLE_MOZ_RIL_GEOLOC) of telephony related code in
> file nsLayoutModule.cpp but not for location. Why is that; wouldn't location
> have the same issue?

Thanks for the feedback, Anshul, I will check the location part.
Flags: needinfo?(echen)
(Assignee)

Comment 15

4 years ago
(In reply to Anshul from comment #13)
> Comment on attachment 8578445 [details] [diff] [review]
> Test Patch, v1
> 
> Review of attachment 8578445 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Edgar, this patch alone, without the need of registering our component as
> gonktelephonyservice seems to have fixed the issue. Seems like you have
> removed featurization (DISABLE_MOZ_RIL_GEOLOC) of telephony related code in
> file nsLayoutModule.cpp but not for location. Why is that; wouldn't location
> have the same issue?

For telephony related IPC stuff, it needs using our own service creator to create *IPCService. But location has different design, so location won't have the same issue. Could you help to test location to double confirm this? If location works as expected, too, I will provide a formal patch and request review then. Thank you.
(Reporter)

Comment 16

4 years ago
Edgar, location is working fine too with your patch.
(Reporter)

Updated

4 years ago
Attachment #8578445 - Flags: feedback?(anshulj) → feedback+
(Assignee)

Updated

4 years ago
Component: Gaia → RIL
(Assignee)

Comment 17

4 years ago
Attachment #8578445 - Attachment is obsolete: true
(Assignee)

Comment 18

4 years ago
Comment on attachment 8580511 [details] [diff] [review]
Patch (part 1), v2

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

Hi Hsinyi, may I have your review? Thank you.
Attachment #8580511 - Flags: review?(htsai)
(Assignee)

Comment 19

4 years ago
Posted patch [b2g37_v2_2] Patch, v1 (obsolete) — Splinter Review
Patch for b2g37_v2.2
(Assignee)

Comment 21

4 years ago
Comment on attachment 8580546 [details] [diff] [review]
[b2g37_v2_2]  Patch, v1

Try result for your reference (b2g37_v2.2):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad269c1bfc15
Posted patch Patch (part 2) (obsolete) — Splinter Review
This is a miss from bug 1091307.
Attachment #8581479 - Attachment description: Patch (v2) → Patch (part 2)
Attachment #8580511 - Attachment description: Patch, v2 → Patch (part 1), v2
A miss from bug 1091307
Attachment #8581479 - Attachment is obsolete: true
Hi Anshul,
I realized that we also need Part2 in addition to Part1. Could you please help verify again for us? Thank you.
Flags: needinfo?(anshulj)
(Reporter)

Comment 25

4 years ago
Hsin-Yi, pulled in both the patches and voice/data are working fine and so is dialer/settings/messaging app.
Flags: needinfo?(anshulj)

Comment 26

4 years ago
Hi Hsin-Yi,

Now that we have positive feedback from CAF is there anything else you need to complete this review and have these changes landed for 2.2?

Thanks,
Mike
Flags: needinfo?(htsai)
(In reply to Mike Lee [:mlee] from comment #26)
> Hi Hsin-Yi,
> 
> Now that we have positive feedback from CAF is there anything else you need
> to complete this review and have these changes landed for 2.2?
> 
> Thanks,
> Mike

Hi Mike,
Even CAF provided positive feedback, we still don't quite understand how the solution could be as it is. I consulted Fabrice who said he was confused by the issue, too. We are still trying to figure the whole situation out to make sure we fix the solution properly. At this moment, I am not comfortable enough to grant the review. Or let me know who knows the best of xpcom registration and bundle mechanism, that person should also be a proper reviewer and I will be happy to help out.
Flags: needinfo?(htsai)
Comment on attachment 8580511 [details] [diff] [review]
Patch (part 1), v2

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

I raised some my concerns to Anshul offline and asked for more tests. The results look good. Though I still have questions about how caf implementation, I don't want to block the ship. So based on the test results, and this patch doesn't harm mozril, I am going to give r+.  
Fabrice, you are the author of bug 1091307, your comments are appreciated.
Attachment #8580511 - Flags: review?(htsai) → review+
Comment on attachment 8581498 [details] [diff] [review]
Patch (part2),  v2

Fabrice, this is somehow missing from bug 1091307. Mind taking a look? Thank you.
Attachment #8581498 - Flags: review?(fabrice)
Comment on attachment 8581498 [details] [diff] [review]
Patch (part2),  v2

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

ok...
Attachment #8581498 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/7f76808f9e49
https://hg.mozilla.org/mozilla-central/rev/4b44c8f09c32
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Posted patch [v2.2] patchSplinter Review
Attachment #8580546 - Attachment is obsolete: true
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Created attachment 8587131 [details] [diff] [review]
> [v2.2] patch

I am waiting for the try result. Once it comes and looks good, I will ask for approval.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #36)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> > Created attachment 8587131 [details] [diff] [review]
> > [v2.2] patch
> 
> I am waiting for the try result. Once it comes and looks good, I will ask
> for approval.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65c17b5f9348 look good
some bustage and errors have been observed with other commits that look unrelated to the patch.
Comment on attachment 8587131 [details] [diff] [review]
[v2.2] patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1091307
User impact if declined: unable to use Dialer, Message apps with vendor ril
Testing completed: yes, based on try tests and CAF verification
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: no
Attachment #8587131 - Flags: approval-mozilla-b2g37?
Attachment #8587131 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.