1091307 - Unable to instantiate Vendor Telephony implementation on FFOS

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Anshul]

This is a regression caused by bug 1072152, which changes the behavior such that an interface registered by the extension with the same name as the internal interface doesn't take precedence anymore.

Comment 1

[Benjamin Smedberg]

When you say "interface registered by the extension" what exactly do you mean? "real" extensions are registered using XRE_AddManifestLocation and ought to take precedence. But I don't think that's what you mean, it seems that you're adding this "extension" as part of the root chrome.manifest. Can you clarify?

Comment 2

[Anshul]

Benjamin, I am no expert in XPCOM so can't answer exactly to your question. We have a chrome.manifest in our component that identifies all the interfaces we expose and expect to override the default interfaces if available with our own implementation.

Comment 3

[Benjamin Smedberg]

Presumably you mean the default implementation? What implementation in particular?

Where is your manifest shipped, and how does it get loaded?

Comment 4

[Anshul]

Our manifest is loaded at /system/b2g/distribution/bundles/telephony on the device. Following are the some of interfaces we expect to override.

@mozilla.org/ril;1
@mozilla.org/telephony/telephonyservice;1
@mozilla.org/cellbroadcast/cellbroadcastservice;1
@mozilla.org/mobileconnection/mobileconnectionservice;1
@mozilla.org/voicemail/voicemailservice;1

Comment 5

[Anshul]

Hello Benjamin, did you get a chance to look at the issue?

Comment 6

[Benjamin Smedberg]

This is an intentional change. GRE omnijar intentionally overrides GRE lists.

To fix this, you should either use an app manifest (instead of a GRE manifest) or explicitly exclude the components that you're overriding from the build.

Overriding components like this is not good engineering in general: ideally you'd fix this by just building the things you need.

Comment 7

[Michael Vines [:m1] [:evilmachines]]

Hey Fabrice, can you please help us bridge the gap here? 

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> 
> ideally you'd fix this by just building the things you need.

Introducing a gecko compile flag to fully exclude the reference Gecko RIL/geolocation components from libxul/omnijar would be one satisfactory way to do this, and save memory on devices that don't require them.

Comment 8

[[:fabrice] Fabrice Desré]

Hi Michael, I'm fine with using a build flag. Let's make it like --disable-mozrilcomponents, and only turned on for your builds,

Comment 9

[Michael Vines [:m1] [:evilmachines]]

Hi Ken, can your team take this bug?  We'll need a --disable-mozgeolocationcomponents flag as well with the build flag approach.

Comment 10

[Ken Chang[:kenkai]]

This flag is only useful for some vendors builds and related to the build procedure.  Shawn, since this flag seems only need to be enabled in specific branch, I wonder if devices team could take this bug or we could have partners' help for this bug.

Comment 11

[Vincent Chang[:vchang][changyihsin]]

Hi there, I think this also block the development of NFC security element. SE function needs QC ril working on m-c to exchange data between UICC and device using iccProvider interface. 

I think we can use version number to reflect interface change. But what's the right way if we have two or more implementations implement the same interface?

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: wip

Michael, can you test this patch on your side?
You'll have to add ac_add_options --disable-mozril-geoloc to your mozconfig.

It's hard for me to test locally because something still tries to use the mobileconnection component and that's causing the startup to fail.

Comment 13

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8546976 [details] [diff] [review]
wip

:D  Anshul, can you please take this for a spin?  "--disable-mozril-geoloc" can be added thusly - https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/b2g_product.mk?h=v2.1#n108

Comment 14

[shawn ku [:sku]]

My two cents,
Shouldn't we separate RIL and Geolcation into two compile flag?


Ni? HsinYi/Edgar to check the dependency of RIL component too.

Comment 15

[Johnny Stenback (:jst)]

Seems Fabrice has this one, reassigning.

Comment 16

[Anshul]

Fabrice, following are my observations.

- On boot up I was expecting a request to instantiate @mozilla.org/ril;1 interfaces but I don't see that happening. I see that should have happened as part of RadioInterfaceLayer.manifest,which you didn't disable with the flag --disable-mozril-geoloc. I see the following error in the logs
"GeckoConsole(  237): While creating services from category 'profile-after-change', could not create service for entry 'RadioInterfaceLayer', contract ID '@mozilla.org/ril;1'". We have our own implementation of '@mozilla.org/ril;1' registered with chrome but don't see even a call to instantiate it.

- I think CellBroadcastService.manifest explicitly instantiate Moz's cellbroadcast component, which may also needs to be disabled.

Comment 17

[Anshul]

Attachment: boot up logs with the proposed patch

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to shawn ku [:sku] from comment #14)
> My two cents,
> Shouldn't we separate RIL and Geolcation into two compile flag?

I'll let you do that in a follow up if you really care. I don't see a strong use case and we're already late.
I also didn't try to figure out which cpp files we could avoid building - I just took care of not registering the components in nsLayoutModule.cpp.


(In reply to Anshul from comment #16)
> Fabrice, following are my observations.
> 
> - On boot up I was expecting a request to instantiate @mozilla.org/ril;1
> interfaces but I don't see that happening. I see that should have happened
> as part of RadioInterfaceLayer.manifest,which you didn't disable with the
> flag --disable-mozril-geoloc. I see the following error in the logs
> "GeckoConsole(  237): While creating services from category
> 'profile-after-change', could not create service for entry
> 'RadioInterfaceLayer', contract ID '@mozilla.org/ril;1'". We have our own
> implementation of '@mozilla.org/ril;1' registered with chrome but don't see
> even a call to instantiate it.

Right, I splitted RadioInterfaceLayer.manifest to only ship RILContentHelper.* now. So I think you need to add the |category profile-after-change RadioInterfaceLayer @mozilla.org/ril;1| in your binary component manifest.

> - I think CellBroadcastService.manifest explicitly instantiate Moz's
> cellbroadcast component, which may also needs to be disabled.

Good catch! fixed in updated patch.

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: wip v2

Anshulj,

can you try this version? Thanks!

Comment 20

[Edgar Chen [:edgar]]

Hi Fabrice,

AFAIK, not only RadioInterfaceLayer and CellBroadcastService but also other ril gonk server will be replaced by partner's implementation.

So I guess we should also disable,
- dom/mobileconnection/gonk/MobileConnectionService.js
- dom/mobileconnection/gonk/MobileConnectionService.manifest
- dom/mobilemessage/gonk/SmsService.js
- dom/mobilemessage/gonk/SmsService.manifest
- dom/telephony/gonk/TelephonyService.js
- dom/telephony/gonk/TelephonyService.manifest
- dom/voicemail/gonk/VoicemailService.js
- dom/voicemail/gonk/VoicemailService.manifest

Hi Anshulj,

Is my understanding correct?

Thank you.

Comment 21

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

(In reply to Fabrice Desré [:fabrice] from comment #18)
> (In reply to shawn ku [:sku] from comment #14)
> > My two cents,
> > Shouldn't we separate RIL and Geolcation into two compile flag?
> 
> I'll let you do that in a follow up if you really care. I don't see a strong
> use case and we're already late.
> I also didn't try to figure out which cpp files we could avoid building - I
> just took care of not registering the components in nsLayoutModule.cpp.
> 
> 

There has been another separate flag MOZ_B2G_RIL to indicate if RIL capability is available, while MOZ_WIDGET_GONK has been for the stuff only available on gonk. Geolocation is one of the latter. From the device point of view, having geolocation doesn't necessarily mean having ril. The current flags do the job well. For this bug, Fabirce suggested we have another flag to indicate if we want to disable mozilla implementation to adopt other implementation even when RIL and Geolocation services are available. I think having separate flags is clear, but I don't know in which case we will want to disable only mozRIL implementation but enable mozGeo implementation, or vice versa. Because AFAIK this newly introduced flag is specifically for the CAF requirement. So I agree with Fabrice that using a single flag looks good enough for me. I don't really have a strong opinion for this.

Instead, my big concern is, is this patch going to be landed on m-c? I am concerned if the answer is yes. I doubt if mozilla is the proper one to maintain the flag on main trunk since we cannot really control which modules and when the modules will/will not be implemented by our partner. That leads to difficulty in maintenance IMO.


> (In reply to Anshul from comment #16)
> > Fabrice, following are my observations.
> > 
> > - On boot up I was expecting a request to instantiate @mozilla.org/ril;1
> > interfaces but I don't see that happening. I see that should have happened
> > as part of RadioInterfaceLayer.manifest,which you didn't disable with the
> > flag --disable-mozril-geoloc. I see the following error in the logs
> > "GeckoConsole(  237): While creating services from category
> > 'profile-after-change', could not create service for entry
> > 'RadioInterfaceLayer', contract ID '@mozilla.org/ril;1'". We have our own
> > implementation of '@mozilla.org/ril;1' registered with chrome but don't see
> > even a call to instantiate it.
> 
> Right, I splitted RadioInterfaceLayer.manifest to only ship
> RILContentHelper.* now. So I think you need to add the |category
> profile-after-change RadioInterfaceLayer @mozilla.org/ril;1| in your binary
> component manifest.
> 
> > - I think CellBroadcastService.manifest explicitly instantiate Moz's
> > cellbroadcast component, which may also needs to be disabled.
> 
> Good catch! fixed in updated patch.

Comment 22

[Michael Vines [:m1] [:evilmachines]]

(In reply to Hsin-Yi Tsai (OOO Jan. 8 ~ Jan. 13) [:hsinyi] from comment #21)
> Instead, my big concern is, is this patch going to be landed on m-c? I am
> concerned if the answer is yes. 

Yes this should also land on m-c for the benefit of future B2G products.

> I doubt if mozilla is the proper one to
> maintain the flag on main trunk since we cannot really control which modules
> and when the modules will/will not be implemented by our partner. That leads
> to difficulty in maintenance IMO.

Reverting the regression in XPCOM loading behaviour on m-c would be another way to resolve this bug, with lower maintenance burden for Mozilla (as has been they way for a couple years now).  Although the freezing of RIL and Geolocation interfaces in v2.2 should mitigate this concern somewhat I'd think.

Comment 23

[Anshul]

(In reply to Edgar Chen [:edgar][:echen] from comment #20)
> Hi Fabrice,
> 
> AFAIK, not only RadioInterfaceLayer and CellBroadcastService but also other
> ril gonk server will be replaced by partner's implementation.
> 
> So I guess we should also disable,
> - dom/mobileconnection/gonk/MobileConnectionService.js
> - dom/mobileconnection/gonk/MobileConnectionService.manifest
> - dom/mobilemessage/gonk/SmsService.js
> - dom/mobilemessage/gonk/SmsService.manifest
> - dom/telephony/gonk/TelephonyService.js
> - dom/telephony/gonk/TelephonyService.manifest
> - dom/voicemail/gonk/VoicemailService.js
> - dom/voicemail/gonk/VoicemailService.manifest
> 
> Hi Anshulj,
> 
> Is my understanding correct?
> 
> Thank you.
Yes Edgar you are correct.

Comment 24

[[:fabrice] Fabrice Desré]

Anshul, can you confirm the full list of xpcom contracts you need to override?

Comment 25

[Anshul]

Fabrice, sorry for the delay, was in a training for last couple of days. I am seeing the following exceptions with your patch.

01-15 02:40:23.000 W/GeckoConsole(  237): createLock@jar:file:///system/b2g/omni.ja!/components/SettingsService.js:267:16
01-15 02:40:23.000 W/GeckoConsole(  237): RadioInterface@jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1686:14
01-15 02:40:23.000 W/GeckoConsole(  237): RadioInterfaceLayer@jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1415:31
01-15 02:40:23.000 W/GeckoConsole(  237): XPCOMUtils__getFactory/factory.createInstance@resource://gre/modules/XPCOMUtils.jsm:292:19
01-15 02:40:23.000 W/GeckoConsole(  237): @jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js:77:13
01-15 02:40:23.000 W/GeckoConsole(  237): XPCU_defineLazyGetter/<.get@resource://gre/modules/XPCOMUtils.jsm:193:33
01-15 02:40:23.000 W/GeckoConsole(  237): RILContentHelper@jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js:126:3
01-15 02:40:23.000 W/GeckoConsole(  237): XPCOMUtils__getFactory/factory.createInstance@resource://gre/modules/XPCOMUtils.jsm:292:19
01-15 02:40:23.000 W/GeckoConsole(  237): " {file: "resource://gre/modules/SettingsRequestManager.jsm" line: 1028}]
01-15 02:40:23.000 I/Gecko   (  237): -*- SettingsRequestManager: Settings queue head blocked at {c6d14106-838b-48f2-bee2-8b87169e12c7} for 1420881127.261 secs, Settings API may be soft lockup. Lock from: SettingsServiceLock@jar:file:///system/b2g/omni.ja!/components/SettingsService.js:80:17
01-15 02:40:23.000 I/Gecko   (  237): createLock@jar:file:///system/b2g/omni.ja!/components/SettingsService.js:267:16
01-15 02:40:23.000 I/Gecko   (  237): RadioInterface@jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1686:14
01-15 02:40:23.000 I/Gecko   (  237): RadioInterfaceLayer@jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js:1415:31
01-15 02:40:23.000 I/Gecko   (  237): XPCOMUtils__getFactory/factory.createInstance@resource://gre/modules/XPCOMUtils.jsm:292:19
01-15 02:40:23.000 I/Gecko   (  237): @jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js:77:13
01-15 02:40:23.000 I/Gecko   (  237): XPCU_defineLazyGetter/<.get@resource://gre/modules/XPCOMUtils.jsm:193:33
01-15 02:40:23.000 I/Gecko   (  237): RILContentHelper@jar:file:///system/b2g/omni.ja!/components/RILContentHelper.js:126:3
01-15 02:40:23.000 I/Gecko   (  237): XPCOMUtils__getFactory/factory.createInstance@resource://gre/modules/XPCOMUtils.jsm:292:19
01-15 02:40:23.000 I/Gecko   (  237): 
01-15 02:40:23.010 W/GeckoConsole(  237): [JavaScript Error: "Settings queue head blocked at {c6d14106-838b-48f2-bee2-8b87169e12c7} for 1420881127.273 secs, Settings API may be soft lockup. Lock from: SettingsServiceLock@jar:file:///system/b2g/omni.ja!/components/SettingsService.js:80:17

Comment 26

[Anshul]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> Anshul, can you confirm the full list of xpcom contracts you need to
> override?
- @mozilla.org/ril;1
- @mozilla.org/telephony/telephonyservice;1
- @mozilla.org/voicemail/voicemailservice;1
- @mozilla.org/cellbroadcast/cellbroadcastservice;1
- @mozilla.org/mobileconnection/mobileconnectionservice;1
- @mozilla.org/sms/smsservice;1

Comment 27

[[:fabrice] Fabrice Desré]

(In reply to Anshul from comment #25)
> Fabrice, sorry for the delay, was in a training for last couple of days. I
> am seeing the following exceptions with your patch.
> 
> 01-15 02:40:23.000 W/GeckoConsole(  237):
> createLock@jar:file:///system/b2g/omni.ja!/components/SettingsService.js:267:
> 16
> 01-15 02:40:23.000 W/GeckoConsole(  237):
> RadioInterface@jar:file:///system/b2g/omni.ja!/components/
> RadioInterfaceLayer.js:1686:14
> 01-15 02:40:23.000 W/GeckoConsole(  237):
> RadioInterfaceLayer@jar:file:///system/b2g/omni.ja!/components/
> RadioInterfaceLayer.js:1415:31

Are you sure you built with the correct flags? (see comment 13). RadioInterfaceLayer.js should not be packaged in your build at all (and it's not in mine).

Comment 28

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Rebased & updated patch, that should exclude everything on the list in comment 26.

Comment 29

[Anshul]

Comment on attachment 8552072 [details] [diff] [review]
patch v3

The patch seems to be working as expected. FYI, the patch needs rebasing as it doesn't apply on the tip of 2.2.

Comment 30

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

Comment on attachment 8552072 [details] [diff] [review]
patch v3

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

Hi Fabrice, thanks for the patch. Regarding my biggest concern in comment 21, I'd like to ask for Jonas' review to confirm if this is the direction we'd like to go. If it's confirmed, the big picture of the patch is okay for the ril part. But according to the list Anshul provided, don't forget that we also need to disable TelephonyService.js and VoicemailService.js  [1], [2].

I am not that familiar to with the geolocation part. But wondering if we need to disable [3], too. Thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/moz.build#66
[2] https://dxr.mozilla.org/mozilla-central/source/dom/voicemail/moz.build#43
[3] https://dxr.mozilla.org/mozilla-central/source/dom/system/moz.build#39

::: b2g/installer/package-manifest.in
@@ +467,3 @@
>  @BINPATH@/components/TelephonyAudioService.js
>  @BINPATH@/components/TelephonyAudioService.manifest
>  @BINPATH@/components/TelephonyService.js

TelephonyService and VoicemailService should be guarded by DISABLE_MOZ_RIL_GEOLOC

::: dom/cellbroadcast/gonk/CellBroadcastService.js
@@ +30,5 @@
>  
> +XPCOMUtils.defineLazyGetter(this, "gRadioInterfaceLayer", function() {
> +  let ril = { numRadioInterfaces: 0 };
> +  try {
> +    ril = Cc["@mozilla.org/ril;1"].getService(Ci.nsIRadioInterfaceLayer);

We would like to apply this to other modules, such as TelephonyService.js, MobileConnectionService.js.

Comment 31

[Mike Lee [:mlee]]

Jonas: Any ETA on reviewing :hsinyi's patch? CAF has a custom gecko patch in place to back bug 1072152 out until fixed and is anxious to have an actual fix so it can be removed.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8552072 [details] [diff] [review]
patch v3

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

Hi Hsin-Yi,

I can't say that I know this code well enough that I can meaningfully mark it r+. I think you should do that instead.

Is the idea here that we enable turning our implementation off for the "@mozilla.org/ril;1" contract. So that a QC XPCOM module can be added which implements that contract instead?

If so that sounds good.

Though it's also important to make sure that that the QC RIL implementation can do everything that it wants to without having to use too many other interfaces. Ideally we would expose some object to the nsIRadioInterfaceLayer object which contains helper functions for all the stuff that the RIL needs to do.

But that's definitely a separate bug.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> Instead, my big concern is, is this patch going to be landed on m-c? I am
> concerned if the answer is yes. I doubt if mozilla is the proper one to
> maintain the flag on main trunk since we cannot really control which modules
> and when the modules will/will not be implemented by our partner. That leads
> to difficulty in maintenance IMO.

This is a good question. Do you have any alternative approaches?

I think it's fine to land on m-c and then adjust what interfaces are turned off based on feedback from partners that ship their own RIL.

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I.e. we don't need to get it perfect from the beginning. We can adjust things as we learn.

Comment 35

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

IMHO the best place for this build flag is vendor repository as I don't see how mozilla will actually ship a build with mozril disabled. However if that doesn't work out for our partners, then I agree we should find a way to improve the current situation. The build flag is not prefect but enough for the case. We will see if we need to make it more flexible. So, based on Jonas' feedback, let go with this direction for now.

Fabrice, please address my comment 30 and ask for review again, thank you very much.

Comment 36

[shawn ku [:sku]]

Fabrice is working on this, remove ni.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hsin-Yi, I think the decision about if we want to land code in m-c or not depends on how many different releases we expect to do with the code, and if we want to maintain the code.

I think it's fine that we aren't doing any releases with this code. The question is if we want to avoid dealing with the complexity of this patch or not.

Generally speaking I would lean towards landing more code, rather than less code in m-c if the code will span multiple releases.

Comment 38

[[:fabrice] Fabrice Desré]

Attachment: patch v4

Hsin-Yi, I addressed your remarks from comment 30. I let the geoloc components since my understanding is that they are fine to keep.

Comment 39

[[:fabrice] Fabrice Desré]

Comment on attachment 8568021 [details] [diff] [review]
patch v4

Anshul, if you can test this one that would be great!

Comment 40

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8568021 [details] [diff] [review]
patch v4

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

::: b2g/installer/package-manifest.in
@@ +397,1 @@
>  @BINPATH@/components/NetworkGeolocationProvider.manifest

This can stay, it's GonkGPSGeolocationProvider.cpp that is replaced (@mozilla.org/gonk-gps-geolocation-provider;1)

Comment 41

[[:fabrice] Fabrice Desré]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #40)
> Comment on attachment 8568021 [details] [diff] [review]
> patch v4
> 
> Review of attachment 8568021 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/installer/package-manifest.in
> @@ +397,1 @@
> >  @BINPATH@/components/NetworkGeolocationProvider.manifest
> 
> This can stay, it's GonkGPSGeolocationProvider.cpp that is replaced
> (@mozilla.org/gonk-gps-geolocation-provider;1)

Alright, I'll change that before landing.

Comment 42

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

Comment on attachment 8568021 [details] [diff] [review]
patch v4

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

Generally looks good to me, thank you. r=me with comment addressed.

Please modify [1] as what you have done for CellBroadcastService.js or MobileConnectionService.js.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js?from=TelephonyService.js#61

Comment 43

[[:fabrice] Fabrice Desré]

Attachment: no-mozril.patch

Final version, that I'd like QA to smoketest before landing.

Comment 44

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8569321 [details] [diff] [review]
no-mozril.patch

Anshul, can you please confirm that this patch looks ok over here as well?  Hopefully the rebase to v2.2 isn't too gnarly, since we aren't running on master ATM.

Comment 45

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

QA tested a locally built custom build and found that the FM radio seem to be busted with the patch.
Testing the inbound build (without the patch) showed that radio works without the patch.

Everything else seem to work ( SMS, Telephony, Data, etc. )

Comment 46

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

It turns out that I ran into the issue without the patch, so there's something else that's going on.  The patch itself seems good.

Comment 47

[Anshul]

Comment on attachment 8569321 [details] [diff] [review]
no-mozril.patch

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

The patch is working as expected. FYI, the FM radio seems to be working fine too for me with this patch.

Comment 48

[[:fabrice] Fabrice Desré]

(In reply to Anshul from comment #47)
> Comment on attachment 8569321 [details] [diff] [review]
> no-mozril.patch
> 
> Review of attachment 8569321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch is working as expected. FYI, the FM radio seems to be working fine
> too for me with this patch.

Yep, it looks like Noaki has some issue with his base image as he confirmed to me that fm radio was broken even without this patch.

Comment 49

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/7abf54d96823

Comment 50

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/b2g-inbound/rev/b36ee59d349e

Comment 51

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please request build peer review for build system changes.

Comment 52

[[:fabrice] Fabrice Desré]

Comment on attachment 8569321 [details] [diff] [review]
no-mozril.patch

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

Mike, can you check the configure.in change? thanks!

Comment 53

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Thanks, Anshul.  I found out that it's a Mac OS X compile bug; I filed bug 1137541
When I compiled on linux, it worked fine for me as well.

Comment 54

[Michael Shal [:mshal]]

Comment on attachment 8569321 [details] [diff] [review]
no-mozril.patch

I might prefer to call it MOZ_RIL_GEOLOC instead of DISABLE_MOZ_RIL_GEOLOC to avoid the double-negative if statements (so it's "if enabled" rather than "if not disabled"), but I don't feel strongly enough to block review. Up to you :)

Comment 55

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/6912175edbf0

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6912175edbf0

Comment 57

[KTucker [:KTucker][Inactive 3/4/2016]]

Removing qawanted since this was already smoke tested.

Comment 58

[Michael Vines [:m1] [:evilmachines]]

It appears somebody backed this out: https://git.mozilla.org/?p=releases/gecko.git;a=commit;h=e705bd42031361eb269f97167d5ec2e04f006aba

Comment 59

[[:fabrice] Fabrice Desré]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #58)
> It appears somebody backed this out:
> https://git.mozilla.org/?p=releases/gecko.git;a=commit;
> h=e705bd42031361eb269f97167d5ec2e04f006aba

We re-landed since.

Comment 60

[Michael Vines [:m1] [:evilmachines]]

Ah, k.  Sry about the spam then.

Comment 61

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 8569321 [details] [diff] [review]
no-mozril.patch

Required for v2.2

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c51b5284fe45