Anshul Reporter
	Description • 9 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}]

	Anshul Reporter
	Comment 1 • 9 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.

	Michael Vines [:m1] [:evilmachines]
	Comment 2 • 9 years ago

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

	Keven Kuo [:kkuo]
	Comment 3 • 9 years ago

Hi! Hsin-Yi,

Could your team help to take a look? Thanks

--
Keven

	shawn ku [:sku]
	Comment 4 • 9 years ago

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

Thanks!!

	[:fabrice] Fabrice Desré
	Comment 5 • 9 years ago

I'll let Hsin-Yi handle that.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 6 • 9 years ago

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?

	Edgar Chen [:edgar] Assignee
	Comment 7 • 9 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

	Anshul Reporter
	Comment 8 • 9 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

	Anshul Reporter
	Comment 9 • 9 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.

	Edgar Chen [:edgar] Assignee
	Comment 10 • 9 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

	Edgar Chen [:edgar] Assignee
	Comment 12 • 9 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).

	Anshul Reporter
	Comment 13 • 9 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?

	Edgar Chen [:edgar] Assignee
	Comment 14 • 9 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.

	Edgar Chen [:edgar] Assignee
	Comment 15 • 9 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.

	Anshul Reporter
	Comment 16 • 9 years ago

Edgar, location is working fine too with your patch.

	Edgar Chen [:edgar] Assignee
	Comment 18 • 9 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.

	Edgar Chen [:edgar] Assignee
	Comment 19 • 9 years ago

Patch for b2g37_v2.2

	Edgar Chen [:edgar] Assignee
	Comment 20 • 9 years ago

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

	Edgar Chen [:edgar] Assignee
	Comment 21 • 9 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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 22 • 9 years ago

This is a miss from bug 1091307.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 23 • 9 years ago

A miss from bug 1091307

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 24 • 9 years ago

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

	Anshul Reporter
	Comment 25 • 9 years ago

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

	Mike Lee [:mlee]
	Comment 26 • 9 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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 27 • 9 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 28 • 9 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 29 • 9 years ago

Comment on attachment 8581498 [details] [diff] [review]
Patch (part2),  v2

Fabrice, this is somehow missing from bug 1091307. Mind taking a look? Thank you.

	[:fabrice] Fabrice Desré
	Comment 30 • 9 years ago

Comment on attachment 8581498 [details] [diff] [review]
Patch (part2),  v2

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

ok...

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 31 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0bb59676176

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 32 • 9 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 33 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 34 • 9 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 36 • 9 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 37 • 9 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 38 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 9 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/74ed9652ce80