1091478 - [Stingray] Refactor the name of TVService to something like TVBackend or so

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Sean Lin [:seanlin] (inactive from Mar 2016)]

We use |nsITVService| and |FakeTVService| in our code base, while actually creating the object with |do_CreateInstance| as opposed to |do_GetService|. It could be kinda misleading or doesn't fit the convention of Gecko to keep naming something which is not a service with 'service' in the name. So it'd be better to refactor this part based on Ehsan's comments.

P.S. The reason why we don't use it as a service (cited from bug 998872 comment 85 and bug 998872 comment 108):

Making an XPCOM service participate in cycle collection is a mistake, since XPCOM services basically live for the lifetime of the process, so if they ever hold a reference to for example a DOM object, we'd be risking holding alive that entire document until shutdown, which is terrible.

If an XPCOM component is instantiated as a service, the XPCOM component manager holds it alive and the next time you call do_GetService it just AddRefs that previous instance and returns it.  (In fact IIRC this is the only actual distinction between XPCOM components and XPCOM services.)  So it is crucial for you to create these objects using do_CreateInstance.  And once you do that, calling the interface a "Service" would be misleading since we reserve that as a convention for things you're supposed to get through do_GetService.

Comment 1

[Marco Chen [:mchen]]

The problem I saw is that there will be hundreds of instances of TVService (depending on number of channels) because each TVChannel create it's dedicated TVService [1]. Refer to other design, the components relationship in each processes would be (A -> B means "A owns B")
  TVManager <-> Listener <- TVService (singleton) <- TVBackend (A new one communicates to real vendor's TV service and provided by vendor as xpcom component)

Then TVManager will declare the cycle between itself and Listner and have a TVManager::Shutdown() in unlink of CC. Once TVManager is freed by CC, the Shutdown() will un-register Listener from TVService. 

Finally we can keep only one TVService and TVBackend in each processes. How about this?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/tv/TVChannel.cpp?from=TVChannel.cpp#70

Comment 2

[(no longer active)]

I'm not sure what question you're asking...  Right now, we create one nsITVService instance per process, right?

Comment 3

[Marco Chen [:mchen]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #2)
> I'm not sure what question you're asking...  Right now, we create one
> nsITVService instance per process, right?

No, currently there are multiple nsITVService per process.
ex: one for each instances of TVManager and one for each instance of TVChannel.

Comment 4

[(no longer active)]

Oh, right, my bad.

But I still don't understand what you're asking.  If you have a way of eliminating the cycles that the TV service participates in, making it a singleton is fine.  But those cycles need to be eliminated, and given the current design of nsITVService, that seems extremely difficult to do.

Comment 5

[Marco Chen [:mchen]]

Hi Ehsan,

Yes, about eliminating the cycle between TVManager and TVService please refer to my proposal as below which is in the description. Or could you show me why this can't solve your concern? Thanks.

---------------
Refer to other design, the components relationship in each processes would be (A -> B means "A owns B")
  TVManager <-> Listener <- TVService (singleton) <- TVBackend (A new one communicates to real vendor's TV service and provided by vendor as xpcom component)

Then TVManager will declare the cycle between itself and Listner and have a TVManager::Shutdown() in unlink of CC. Once TVManager is freed by CC (calling unlink), the Shutdown() will un-register Listener from TVService. 
-------------------

Comment 6

[(no longer active)]

Can Listener objects hold a reference to TVService in this design?  If yes, then the same cycle issue that I was worried about would still exist.  If not, then your design seems fine from my perspective.

Also please note that there are parts of the existing design such s nsITVSourceListener that would need to be completely replaced with another mechanism, since the TVBackend is supposed to not hold references to its consumers.

Comment 7

[Marco Chen [:mchen]]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #6)
> Can Listener objects hold a reference to TVService in this design?  

The answer is no.
Referring to comment 5, only TVService holds a reference to Listner.

> Also please note that there are parts of the existing design such s
> nsITVSourceListener that would need to be completely replaced with another
> mechanism, since the TVBackend is supposed to not hold references to its
> consumers.

Right and thanks.

Comment 8

[BMO Automation]

Firefox OS is not being worked on