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}]

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.

(2.2+, as this needs to be fixed to commercialize v2.2)

Hi! Hsin-Yi, Could your team help to take a look? Thanks -- Keven

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

I'll let Hsin-Yi handle that.

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?

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

(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

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.

(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

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

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?

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

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

Edgar, location is working fine too with your patch.

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.

Patch for b2g37_v2.2

Comment on attachment 8580511 [details] [diff] [review] Patch (part 1), v2 Try result for your reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41423bd797ad&exclusion_profile=false

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

This is a miss from bug 1091307.

A miss from bug 1091307

Hi Anshul, I realized that we also need Part2 in addition to Part1. Could you please help verify again for us? Thank you.

Hsin-Yi, pulled in both the patches and voice/data are working fine and so is dialer/settings/messaging app.

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

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

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.

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

Comment on attachment 8581498 [details] [diff] [review] Patch (part2), v2 Review of attachment 8581498 [details] [diff] [review]: ----------------------------------------------------------------- ok...

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #31) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0bb59676176 https://treeherder.mozilla.org/#/jobs?repo=try&revision=68de9db5d164&exclusion_profile=false after fixing some stupid error, looking good.

https://hg.mozilla.org/integration/b2g-inbound/rev/7f76808f9e49 https://hg.mozilla.org/integration/b2g-inbound/rev/4b44c8f09c32

https://hg.mozilla.org/mozilla-central/rev/7f76808f9e49 https://hg.mozilla.org/mozilla-central/rev/4b44c8f09c32

(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