786468 - Support alternate nsIRadioInterfaceLayer implementations

Status: RESOLVED FIXED

Description

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

Enhance SystemWorkerManager to enable runtime selection of the nsIRadioInterfaceLayer implementation based on something like a system property or config file.

Comment 1

[Philipp von Weitershausen [:philikon]]

I *think* you should get this for free from XPCOM by simply registering a different implementation for the same CID (class ID). It also depends a bit on what you mean by "runtime".

Can you elaborate a bit on the use case? Swapping out all of nsIRadioInterfaceLayer seems like a big hammer. Maybe there's a more fine-grained solution?

Comment 2

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

By runtime I mean "via something external to the gecko binaries" (ie, an extension).  The intent is to not get in the way of gecko updates but enable an external nsIRadioInterfaceLayer implementation.   I'm so not an XPCOM expert but seems like an extension registering as implementing NS_RADIOINTERFACELAYER_CID would result in undefined behavior -- maybe XPCOM picks up the extension's implementation while enumerating classes but maybe not.

Comment 3

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

I just tried adding {2d831c8d-6017-435b-a80ce5d422810cea} (NS_RADIOINTERFACELAYER_CID) to the extension's chrome.manifest and XPCOM still picked up the in-gecko implementation.  Not surprising at all but just wanted to confirm.

Comment 4

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

Created attachment 656315 [details] [diff] [review]
attempt #1

Alternate nsIRadioInterfaceLayer implementation can be selected by setting the "ro.moz.ril.contractid" property.

https://tbpl.mozilla.org/?tree=Try&rev=cb97c688596e

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

A CID ("class ID") refers to a particular implementation class so you can never override a previously registered CID. You want to override the "contract ID" instead.

Comment 6

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

A contract ID is not currently defined for nsIRadioInterfaceLayer, just a CID [1].  But if we were to define a contract ID for this interface, @mozilla.org/ril/radiointerfacelayer;1 or something, an extension could override it via its chrome.manifest?  Seems like this would have the same ambiguity problem during XPCOM component enumeration (but again, I am *not* an XPCOM expert).   

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/SystemWorkerManager.cpp#327

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yep, you'll need to register a contract ID for the RIL, then switch the createInstance call in SystemWorkerManager.cpp to use the contract ID instead of a CID. Then an extension can override the contract ID and will get created instead.

The only issue will be that the extension's contract ID override happens slightly after startup (if we call createInstance before extension registration then this won't work). I think it should be ok since we initialize the SystemWorkerManager (and then the RIL) at "profile-after-change" ( http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1232 ).

Not sure what you mean by the "ambiguity problem during XPCOM component enumeration"... Think of the component manager as a big hash table mapping contract IDs to class factories (that's really all it is). By overriding the contract ID you're just replacing the old factory with a different factory.

Comment 8

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

Awesome, that sounds much nicer.  I'll give that a go.

Comment 9

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

Created attachment 656734 [details] [diff] [review]
attempt #2 -- using a contract ID

This is much nicer now with the system property junk removed.  Maybe the contact ID name as a little weak: "@mozilla.org/ril;1".  Not attached to it by any means so suggestions welcome!

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 656734 [details] [diff] [review]
attempt #2 -- using a contract ID

This looks great! Though, I wonder why you need to switch nsIWorkerHolder to nsISupports here?

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to ben turner [:bent] from comment #10)
> This looks great! Though, I wonder why you need to switch nsIWorkerHolder to
> nsISupports here?

Presumably so that alternate implementations of nsIRadioInterfaceLayer don't have to also implement nsIWorkerHolder, e.g. if they don't want to use ChromeWorkers.

Comment 12

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

Yep, that's correct.  Can I ask you or bent for a r?  I'll push this to try later today, but I don't expect any issues given no significant change since the last try run.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, then you would want it to be nsIRadioInterfaceLayer, not nsISupports.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The other thing is we'll have to be careful with this change. Currently consumers of nsIRadioInterfaceLayer have to get the system worker manager service and then call do_GetInterface on it. If we actually have a separate component that we want to use then we should probably change the createInstance call to getService, otherwise other consumers could just use the ril contractid to create multiple instances of the new component.

Comment 15

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

Created attachment 656948 [details] [diff] [review]
attempt #3 -- use nsIRadioInterfaceLayer not nsISupports.

also rebased to address patch rot

Comment 16

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

Good point about using nsIRadioInterfaceLayer, thanks.

(In reply to ben turner [:bent] from comment #14)
> If we actually have a separate
> component that we want to use then we should probably change the
> createInstance call to getService, otherwise other consumers could just use
> the ril contractid to create multiple instances of the new component.

I'm not thinking of any changes to the consumers, they would continue going through swm.  Although I see how declaring a contractid makes it a little more likely that somebody in the future would attempt to CI the ril directly and wonder why it didn't work as expected.  Is that a risk we can accept for now (maybe until the 1st or 2nd person stubs their toe)?

Comment 17

[Philipp von Weitershausen [:philikon]]

The reason for the current setup is that SWM is in charge of the RIL. In particular, it tries to ensure nobody else creates the RIL first, so that the SWM gets the chance to attach its things to the worker in time.

So yes, I agree about not changing consumers. I would be ok with a big fat comment in the manifest to let people know to not use the contract id to acquire the RIL (instead tell them how else to do it).

Comment 18

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

How about this for the comment?  I'll upload a new patch once we get the wording right:

# RadioInterfaceLayer.js
#
# IMPORTANT:
# Consumers of nsIRadioInterfaceLayer should invoke
# nsIInterfaceRequestor::GetInstance() as implemented by
# "@mozilla.org/telephony/system-worker-manager;1" to
# obtain an instance.
#
# DO NOT use do_CreateInstance()/do_GetService() to directly
# instantiate "@mozilla.org/ril;1".
#

Comment 19

[Philipp von Weitershausen [:philikon]]

(In reply to Michael Vines [:m1] from comment #18)
> # IMPORTANT:
> # Consumers of nsIRadioInterfaceLayer should invoke
> # nsIInterfaceRequestor::GetInstance() as implemented by

itym nsIInterfaceRequestor::GetInterface(). sgtm otherwise

Comment 20

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

Created attachment 657021 [details] [diff] [review]
attempt #4 -- += contract ID usage disclaimer

Comment 21

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

Try run: https://tbpl.mozilla.org/?tree=Try&rev=65a80fc99720

(Looks like I ended up pushing two unrelated inbound patches to try in addition to this patch as well.  A little unhygienic but builds are looking green so far regardless)

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 657021 [details] [diff] [review]
attempt #4 -- += contract ID usage disclaimer

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

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +472,1 @@
>    }

I am not qualified to rubberstamp this refcounting black magic. mrbkap, is this ok?

lgtm otherwise.

Comment 23

[Blake Kaplan (:mrbkap)]

Comment on attachment 657021 [details] [diff] [review]
attempt #4 -- += contract ID usage disclaimer

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

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +472,1 @@
>    }

Yeah, this is fine.

@@ +498,3 @@
>  
> +  nsCOMPtr<nsIWorkerHolder> worker = do_QueryInterface(ril);
> +  if (worker != 0) {

Please don't compare with 0, just |if (worker)|.

Comment 24

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

Created attachment 657327 [details] [diff] [review]
attempt #5 -- address trivial style issue

Trivial change from previous patch to address |if (worker)| coding style nit.

Comment 25

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb57e4e09bb

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfb57e4e09bb