Support alternate nsIRadioInterfaceLayer implementations

RESOLVED FIXED in mozilla18

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: m1, Assigned: m1)

Tracking

unspecified
mozilla18
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Enhance SystemWorkerManager to enable runtime selection of the nsIRadioInterfaceLayer implementation based on something like a system property or config file.
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?
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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
Attachment #656315 - Flags: review?(philipp)
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.
(Assignee)

Comment 6

5 years ago
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
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.
(Assignee)

Comment 8

5 years ago
Awesome, that sounds much nicer.  I'll give that a go.
(Assignee)

Updated

5 years ago
Attachment #656315 - Attachment is obsolete: true
Attachment #656315 - Flags: review?(philipp)
(Assignee)

Comment 9

5 years ago
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 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?
(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.
(Assignee)

Comment 12

5 years ago
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.
Hm, then you would want it to be nsIRadioInterfaceLayer, not nsISupports.
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.
(Assignee)

Comment 15

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

also rebased to address patch rot
Attachment #656734 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
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)?
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).
(Assignee)

Comment 18

5 years ago
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".
#
(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
(Assignee)

Comment 20

5 years ago
Created attachment 657021 [details] [diff] [review]
attempt #4 -- += contract ID usage disclaimer
Attachment #656948 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
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)
(Assignee)

Updated

5 years ago
Attachment #657021 - Flags: review?(philipp)
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.
Attachment #657021 - Flags: review?(philipp)
Attachment #657021 - Flags: review?(mrbkap)
Attachment #657021 - Flags: review+
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)|.
Attachment #657021 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 24

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

Trivial change from previous patch to address |if (worker)| coding style nit.
Attachment #657021 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb57e4e09bb
Component: General → DOM: Device Interfaces
Keywords: checkin-needed
Product: Boot2Gecko → Core
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/bfb57e4e09bb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.