Last Comment Bug 786468 - Support alternate nsIRadioInterfaceLayer implementations
: Support alternate nsIRadioInterfaceLayer implementations
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla18
Assigned To: Michael Vines [:m1] [:evilmachines]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-28 15:26 PDT by Michael Vines [:m1] [:evilmachines]
Modified: 2012-08-31 18:43 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
attempt #1 (3.70 KB, patch)
2012-08-28 20:16 PDT, Michael Vines [:m1] [:evilmachines]
no flags Details | Diff | Splinter Review
attempt #2 -- using a contract ID (3.49 KB, patch)
2012-08-29 20:41 PDT, Michael Vines [:m1] [:evilmachines]
no flags Details | Diff | Splinter Review
attempt #3 -- use nsIRadioInterfaceLayer not nsISupports. (4.50 KB, patch)
2012-08-30 10:58 PDT, Michael Vines [:m1] [:evilmachines]
no flags Details | Diff | Splinter Review
attempt #4 -- += contract ID usage disclaimer (4.84 KB, patch)
2012-08-30 14:01 PDT, Michael Vines [:m1] [:evilmachines]
philipp: review+
mrbkap: review+
Details | Diff | Splinter Review
attempt #5 -- address trivial style issue (4.83 KB, patch)
2012-08-31 09:41 PDT, Michael Vines [:m1] [:evilmachines]
no flags Details | Diff | Splinter Review

Description Michael Vines [:m1] [:evilmachines] 2012-08-28 15:26:41 PDT
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] 2012-08-28 15:41:53 PDT
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] 2012-08-28 16:27:35 PDT
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] 2012-08-28 17:20:58 PDT
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] 2012-08-28 20:16:03 PDT
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!) 2012-08-28 21:00:10 PDT
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] 2012-08-28 21:09:32 PDT
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!) 2012-08-28 21:25:05 PDT
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] 2012-08-28 21:29:15 PDT
Awesome, that sounds much nicer.  I'll give that a go.
Comment 9 Michael Vines [:m1] [:evilmachines] 2012-08-29 20:41:35 PDT
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!) 2012-08-30 05:28:15 PDT
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] 2012-08-30 07:26:56 PDT
(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] 2012-08-30 08:08:35 PDT
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!) 2012-08-30 08:34:22 PDT
Hm, then you would want it to be nsIRadioInterfaceLayer, not nsISupports.
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-30 08:37:13 PDT
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] 2012-08-30 10:58:29 PDT
Created attachment 656948 [details] [diff] [review]
attempt #3 -- use nsIRadioInterfaceLayer not nsISupports.

also rebased to address patch rot
Comment 16 Michael Vines [:m1] [:evilmachines] 2012-08-30 11:11:14 PDT
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] 2012-08-30 11:27:00 PDT
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] 2012-08-30 13:43:28 PDT
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] 2012-08-30 13:47:45 PDT
(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] 2012-08-30 14:01:09 PDT
Created attachment 657021 [details] [diff] [review]
attempt #4 -- += contract ID usage disclaimer
Comment 21 Michael Vines [:m1] [:evilmachines] 2012-08-30 15:07:50 PDT
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] 2012-08-31 07:33:52 PDT
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) 2012-08-31 07:59:50 PDT
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] 2012-08-31 09:41:12 PDT
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] 2012-08-31 11:01:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb57e4e09bb
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-08-31 18:43:56 PDT
https://hg.mozilla.org/mozilla-central/rev/bfb57e4e09bb

Note You need to log in before you can comment on or make changes to this bug.