634229 - Implementation of a services module

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Similar to the Services module in Firefox 4 we need a module which offers instances for different core services and interfaces. I will collect our mostly used instances and build-up a services module.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The services module should look similar to the one we have in Firefox 4. It has to offer support for often used services and interfaces. The following URL will demonstrate that:

resource://gre/modules/Services.jsm

Given our former discussions that we do not want to update our older branches anymore except fixing broken tests, I wonder if we should simply use the global services module Firefox 4 provides.

Heather, do you know if the securable modules support some form of caching for such URLs similar to what Firefox does? Such a JS module should really only be loaded once. IMO running a couple of tests, which all include that module, could cause leaks. Do we know anything of that? Or will we have to ask Atul?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #1)
> Heather, do you know if the securable modules support some form of caching for
> such URLs similar to what Firefox does? Such a JS module should really only be
> loaded once. IMO running a couple of tests, which all include that module,
> could cause leaks. Do we know anything of that? Or will we have to ask Atul?

Atul, can you give us a bit insight how securable modules handle that?

Comment 3

[Atul Varma [:atul]]

Sure.

Basically, every require() function is associated with a "module loader" instance. Every time you require('foo') from the same module loader (e.g., the same require() function), you're getting the exact same module instance--the module's source code is never reloaded from disk and re-evaluated.

However, you can also create new "module loader" instances, if you want. Among other things, we actually do this in Jetpack's testing framework so we can "supervise" one loader's memory use from another loader. We also do it in some development tools like "Cuddlefish Lab", where we want to be able to quickly make a change to a securable module and try out the change without having to restart the whole browser.

Also, in general, because each jetpack-created XPI is completely self-contained--similar to a statically-linked executable--they each run with their own module loader instance. That means that a require('foo') in add-on A will *not* return the same thing as a require('foo') in add-on B. The two 'foo' modules are loaded side-by-side, and actually aren't aware of one another's existence.

Does that answer your question?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, it does indeed. Thanks for your reply. The module loader in our case is part of Mozmill itself, and our tests always use the same one. That means that such a services module will be shared between the different tests.

Comment 5

[cmtalbert]

Wait, you're replacing the standard gre Services.jsm with a QA-created one?

Or you're only doing that for branches that do not have a Services.jsm?  I'm not real clear on what problem you're solving here and how you're doing it.

I would strongly encourage you to not replace the services.jsm for branches that already have a services.jsm.  Because if you do that you run the real risk of breaking in future versions as implementations of the services.jsm are changed.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, I do not really want to replace it, but we need a simpler way of using those services. At the moment each test would have to import the global Services.jsm file before any service can be used. What I want to do is to have a wrapper which hides that and augments it automatically to the module object.

Because the refactoring of shared mods will not be backported to older branches, we should be safe in using the global services.jsm. We simply have to make it accessible for test creators in an easy way.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Atul, I have a strange behavior when trying to Cu.import the Services module. In the case as given below it is undefined. Normally when importing it into the global namespace there should be a Services object present. 

Cu.import('resource://gre/modules/Services.jsm');
dump("** dump: " + Services + "\n");


Is there any reason why that fails by using securable modules?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, so it seems that it is related to the sandbox. We define Cu in Mozmill and export that for our tests. I can also Cu.import Mozmill specific js modules, but it fails when trying to import global js modules.

When I'm using Components.utils.import the import works as expected. That's kinda strange but no regression in Firefox 4. I can see the same problems with Namoroka. So I will keep the long name for now. Weird.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Geo, if you have time later please check the first push of the services module. You can find it here:

https://github.com/geoelectric/mozmill-api-refactor/commit/6930b8d943cb82fb60203b1da111335d48ce6a8e

My question is how we want to handle services which are used often in specific modules, i.e. sessionStore, places stuff, and so on. Not sure if we should add everything to the services module. I would be more inclined to inject that in the modules itself. What do you think? Should we overload the services module?

Comment 10

[Atul Varma [:atul]]

Oh, sorry about that... Unfortunately, I have no idea why, but the version of Cu.import() without the second optional parameter doesn't appear to work in Cu.Sandbox objects in general (I think that the calls don't work in the bootstrap.js environment of restartless extensions either, but I could be mistaken).

Because of this, all Jetpack code actually uses the second optional parameter to contain the exports of the jsm, e.g. your code becomes:

  var jsm = {};
  Cu.import('resource://gre/modules/Services.jsm', jsm);
  dump("** dump: " + jsm.Services + "\n");

If you find a way around this weird bug, though, let me know!

Comment 11

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #9)
> Geo, if you have time later please check the first push of the services module.
> You can find it here:
> 
> https://github.com/geoelectric/mozmill-api-refactor/commit/6930b8d943cb82fb60203b1da111335d48ce6a8e
> 
> My question is how we want to handle services which are used often in specific
> modules, i.e. sessionStore, places stuff, and so on. Not sure if we should add
> everything to the services module. I would be more inclined to inject that in
> the modules itself. What do you think? Should we overload the services module?

Well, TBH, I thought the whole purpose of such a module is to keep things like...

var count = services.get("@mozilla.org/browser/sessionstore;1", Ci.nsISessionStore)

...out of tests. The "unit test" code you replaced looks much less clear now, for example. I liked what we had there before -far- better.

So yeah, I'm a little bummed by that step backwards. To me the big reason to have a services module was to get rid of that. 

Re: putting them in other modules, I think for our specific purpose it's better to have a module that represents getting all platform services. Reason is that we're unlikely to have (for example) a session store module otherwise. There are a lot of back end services that don't correspond to modules we'd be likely to need for front-end functionality.

Re: Services.jsm, longer discussion here:

I'm not sure about just renaming stuff from Services.jsm. That approach has pros and cons. Pros: the names you've chosen are certainly more descriptive, Cons: now they're proprietary to our code, so devs have to learn a whole new set of identifiers.

Now that I see that Services.jsm can be just straight imported, I'm more likely to say that just should be done by modules that can use them. Can we do something like:

function importPlatformServices() {
  Components.utils.import('resource://gre/modules/Services.jsm');
}

...and then have shared mods (or tests, I guess) that need them call services.importPlatformServices() at the top of their module? If that import would only bring Services into function scope when called that way, maybe it'd require something like:

function importPlatformServices() {
  Components.utils.import('resource://gre/modules/Services.jsm');
  return Services;
}

and then Services = services.importPlatformServices() in modules that needed it.

The point is to get the technical aspects of the import out of repeated code. We might just be able to make the resource url a module constant too (imagine a module called consts) and do:

Component.utils.import(consts.SERVICE);

...where needed.

Maybe we should have a short conversation tomorrow re: what direction to take this module. It'd be easier to talk about design aspects in that mode rather than feel like I'm calling for complete code rewrites.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #10)
> Because of this, all Jetpack code actually uses the second optional parameter
> to contain the exports of the jsm, e.g. your code becomes:
> 
>   var jsm = {};
>   Cu.import('resource://gre/modules/Services.jsm', jsm);
>   dump("** dump: " + jsm.Services + "\n");
> 
> If you find a way around this weird bug, though, let me know!

Atul, that will not work either. The problem for me simply was Cu.import vs. Components.utils.import. The latter solution solved the problem for me. Just see my comment 8. We still think that this is strange.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #11)
> Well, TBH, I thought the whole purpose of such a module is to keep things
> like...
> 
> var count = services.get("@mozilla.org/browser/sessionstore;1",
> Ci.nsISessionStore)
> 
> ...out of tests. The "unit test" code you replaced looks much less clear now,
> for example. I liked what we had there before -far- better.

That's why I have asked you that question. The example below is not a general service, but a service bounded to the session restore component. General services like appInfo can be accessed via services.appInfo.

> Re: putting them in other modules, I think for our specific purpose it's better
> to have a module that represents getting all platform services. Reason is that
> we're unlikely to have (for example) a session store module otherwise. There
> are a lot of back end services that don't correspond to modules we'd be likely
> to need for front-end functionality.

All services will be lazily loaded, like the one for prefs I have included at the beginning of the services module. So it will not hurt the initial loading, even with a lot of services included in that file.

> I'm not sure about just renaming stuff from Services.jsm. That approach has
> pros and cons. Pros: the names you've chosen are certainly more descriptive,
> Cons: now they're proprietary to our code, so devs have to learn a whole new
> set of identifiers.

We can keep the same name, I don't have a problem with. The only thing we have to do is to reassign names to internal representations, because I don't want to have a dependency for our tests to the external Services.jsm. If some parts will change in the future we would have to update all of our tests. That's why user defined names for us make sense.

> function importPlatformServices() {
>   Components.utils.import('resource://gre/modules/Services.jsm');
> }

We don't have to do that. Everything gets lazily loaded already.

Comment 14

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Not sure why you think I'm concentrating on lazy-loading; I'm not. My suggestions were tilted towards keeping:

Components.utils.import('resource://gre/modules/Services.jsm')

...out of files, without having to singly export each identifier in it through a proxy module. So I'm wondering if we can encapsulate the import in an easier-to-read function call.

Also, I'm unclear what our conclusion is with the encapsulating the services.get stuff.

Good to know that there are no performance concerns here, though.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #14)
> Components.utils.import('resource://gre/modules/Services.jsm')
> 
> ...out of files, without having to singly export each identifier in it through
> a proxy module. So I'm wondering if we can encapsulate the import in an
> easier-to-read function call.

No we can't because we only have those two options:

* Create our own lazy loading function calls and don't use the gre version
* Re-assign the Services function names to names we use in our tests

I wouldn't go the first way because there is no reason to re-define everything from scratch. There are two pro's for the second path:

1. If the services.jsm gets updated and property names changed we only have to update our services module and not all of the tests
2. We can add proper documentation for each of the supported services by this module.

> Also, I'm unclear what our conclusion is with the encapsulating the
> services.get stuff.

As said I'm happy to put all services we are using in the same file.

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

OK, thanks for clarifications. As we discussed on the call, I'm not too worried about services.jsm changing (too much hanging off of it) but it is more straightforward to document this way. Let's go with what you have.

Comment 17

[Atul Varma [:atul]]

Hmm, that's particularly weird that the providing-an-exports-object variant of Cu.import() doesn't work... In that case, I have no idea what is going on.

The one thing I was thinking of was that perhaps every Sandbox's Components.utils object is tied to the global scope that it came from. If that's the case, then the securable-module implementation may actually be giving a single scope's Components.utils object to every securable-module that's created, which would result in a bunch of things being exported to a single global scope that none of the securable modules can access. Hmm.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #17)
> The one thing I was thinking of was that perhaps every Sandbox's
> Components.utils object is tied to the global scope that it came from. If

Well, that's something I have completely forgotten about. So it could be that it is related to how securable modules are handling Cc, Ci, Cu,... ? Should I file a new bug for this case? If yes, which product / component I would have to use?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: WIP

First iteration of possible services we are using often in our current modules and tests. If you agree on it Geo, I will move forward. I would even favorite our own Services class similar to assertions, which should make it easier for anyone later to own Services with additions, i.e. in the add-ons tests. What do you think?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #19)
> and tests. If you agree on it Geo, I will move forward. I would even favorite
> our own Services class similar to assertions, which should make it easier for

Lets skip this part for now and simply have global methods in this module. We can extend that later on if required.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I need the assertion module landed before I can start creating tests for this module.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1

First version of the patch with documentation but without tests. I need the assertion module landed first to get tests implemented.

Comment 23

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 515585 [details] [diff] [review]
Patch v1

Docs:

(init.js)

+  * @name services
+  * @see services
+  * @memberOf module

This didn't do what you thought it was going to. No services member shows up in the module namespace, much less with a link to the services namespace. You may need to identify it as a @field, remove the @see, something. Unsure what.

(services.js)

+/**
+ * @name services
+ * @namespace 
+ */

Please give the namespace a description (@namespace "some description") so it appears in the class index.

All the services fields come in as "static." Not sure if we prefer that or not; technically true, but kinda superfluous since it's a namespace and not a class. 

If we don't want them, you can probably get those to go away with a @memberOf services# (note the # to denote instance). You can likely stick that in a metatag (see the jsdoc wiki) just under the namespace to get it to automatically apply to all members in the file w/ jsdoc info.

Otherwise, looks fine. Tests do need to be added, but I don't care if it's on this branch or during "refactoring." Also not 100% sure how you'll test the services--hopefully just something simple like they don't come out undefined?

Re: your dependency, you're more than welcome to go ahead and merge both branches into master tonight if you want. Just drop me a note if you do so I'm not confused.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #23)
> (init.js)
> 
> +  * @name services
> +  * @see services
> +  * @memberOf module
> 
> This didn't do what you thought it was going to. No services member shows up in
> the module namespace, much less with a link to the services namespace. You may
> need to identify it as a @field, remove the @see, something. Unsure what.

That's because you will need the patch for the assertions module. Given the patch in that form it will not work.

> Please give the namespace a description (@namespace "some description") so it
> appears in the class index.

Just missed to add in the last patch.

> All the services fields come in as "static." Not sure if we prefer that or not;
> technically true, but kinda superfluous since it's a namespace and not a class. 

Not sure why it is not working for you. Using the outline template all those methods are listed below the services module. But I don't have a problem to use the metatag field. Updated.

> services--hopefully just something simple like they don't come out undefined?

That's what I have in mind. We can't really do more and probably shouldn't. If we get an undefined for a well-known issue, Mochitests have been failed.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v2

Final version with updated docs and tests to ensure all services are available.

Comment 26

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 515934 [details] [diff] [review]
Patch v2

Looks good, thanks for the changes.

re: the static thing, the fields themselves came out fine in the defaults template too; the fields were just marked as static members of the service namespace. Static/dynamic just doesn't make a lot of sense in the context of namespaces.

My guess is in that outline template they either don't list static/dynamic, or it's one of the little icons somewhere, so you didn't see or notice the problem.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/3d59b6d9e072501b44fc2e3d89a6fdd9854d405e