562819 - Make Jetpack-generated XPIs rebootless

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Atul Varma [:atul]]

For the rebootless spec:

  https://wiki.mozilla.org/Extension_Manager:Bootstrapped_Extensions

An example of a trivial rebootless extension:

  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/addons/test_bootstrap1_1/

Comment 1

[Atul Varma [:atul]]

Attachment: Preliminary patch

Comment 2

[Atul Varma [:atul]]

This patch seems to work okay when installing a cfx-generated XPI after Firefox has started up.

However, 'cfx test -a firefox' raises NS_ERROR_FAILURE when trying to call nsIWindowWatcher.openWindow(). Presumably, this is because my patch doesn't actually attach an observer for "sessionstore-windows-restored" or "final-ui-startup" the way that the XPCOM harness does, so the test harness is trying to open a window before the application is actually able to.

One weird thing about installing cfx-generated XPIs after Firefox has started up, though, is that two entries appear in the addons manager for the addon: one implies that the addon is already installed (which it is), but the other is an entry for the same addon with a button labeled "Restart now", which seems to imply that the addon is a legacy non-restartless addon.

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #2)
> One weird thing about installing cfx-generated XPIs after Firefox has started
> up, though, is that two entries appear in the addons manager for the addon: one
> implies that the addon is already installed (which it is), but the other is an
> entry for the same addon with a button labeled "Restart now", which seems to
> imply that the addon is a legacy non-restartless addon.

This is an add-ons manager bug,should be fixed by bug 563562

Comment 4

[Atul Varma [:atul]]

Ugh, it looks like I may have accidentally pushed attachment 442580 [details] [diff] [review] as part of a different patch:

  http://hg.mozilla.org/labs/jetpack-sdk/rev/c18a103886e3

Grr.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #4)
> Ugh, it looks like I may have accidentally pushed attachment 442580 [details] [diff] [review] as part of
> a different patch:
> 
>   http://hg.mozilla.org/labs/jetpack-sdk/rev/c18a103886e3
> 
> Grr.

I backed it out for you :)
http://hg.mozilla.org/labs/jetpack-sdk/rev/d5582d613650

Comment 6

[Atul Varma [:atul]]

Thanks Paul!

Comment 7

[Dietrich Ayala (:dietrich)]

Hey Atul, is this still on-track for code freeze tomorrow? Even without Lifecycle, having this work would make 0.4 very sweet.

Comment 8

[Atul Varma [:atul]]

Yep, I'm trying to get this landed by tomorrow night.

Right now cfx run/test seems blocked by bug 566485, but we can potentially get around that if it's not fixable soon by having the <em:bootstrap>true</em:bootstrap> only be present when doing 'cfx xpi'.

Comment 9

[Atul Varma [:atul]]

Attachment: latest WIP patch

Comment 10

[Atul Varma [:atul]]

Comment on attachment 445857 [details] [diff] [review]
latest WIP patch

Mossop, can you take a quick look at this and let me know what you think? Specifically, I'd like to know what you think about the TODOs.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 445857 [details] [diff] [review]
latest WIP patch

Looks good, a few quick questions, but seems ok anyway.

>+var manager = Components.manager;
>+manager.QueryInterface(Ci.nsIComponentRegistrar);

Can just do |var manager = Components.manager.QueryInterface(Ci.nsIComponentRegistrar)|

>+// TODO: Not sure if we should actually just let the error
>+// propagate through to the extension manager code here or
>+// not.

The extension manager will log any errors to the error console and if extensions.logging.enabled is on to stdout, up to you if you need anything more.

>+  var factory = HarnessService.prototype._xpcom_factory;
>+  var proto = HarnessService.prototype;
>+
>+  manager.registerFactory(proto.classID,
>+                          proto.classDescription,
>+                          proto.contractID,
>+                          factory);
>+  
>+  var harnessService = factory.createInstance(null, Ci.nsISupports);
>+  harnessService = harnessService.wrappedJSObject;

Is there a need to register this here? Can't you just do |HarnessService()| and avoid xpcom completely? If not can you unregister the factory immediately?

>+function install(data, reason) {
>+  // TODO: Should we only start up the extension on startup(), and never
>+  // here?
>+  safeSetupHarness(data.installPath);

Yeah you shouldn't start up here, startup will always be called when an extension should load. install may be called even if an extension is disabled right now.

>+}
>+
>+function startup(data, reason) {
>+  safeSetupHarness(data.installPath);
>+}
>+
>+function shutdown(data, reason) {
>+  safeShutdownHarness();
>+}
>+
>+function uninstall(data, reason) {
>+  // TODO: Should we only shut down the extension on shutdown(), and
>+  // never here?
>+  safeShutdownHarness();

And shouldn't shut down here, uninstall may be called even when startup has not been called.

Comment 12

[Atul Varma [:atul]]

Thanks for the feedback! I will implement all your suggestions, with the possible exception of the harnessService issue: this is actually how traditional extensions are told to access the jetpack-based parts of their extension:

  https://jetpack.mozillalabs.com/sdk/0.3/docs/#guide/xul-extensions

That said, since rebootless extensions can't actually use XUL overlays or have chrome manifests, there may not be much of a use case for this.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> Thanks for the feedback! I will implement all your suggestions, with the
> possible exception of the harnessService issue: this is actually how
> traditional extensions are told to access the jetpack-based parts of their
> extension:

Ok that's fine, just couldn't see an obvious reason for it in the patch.

> That said, since rebootless extensions can't actually use XUL overlays or have
> chrome manifests, there may not be much of a use case for this.

We're potentially going to add support for registering chrome namespaces for rebootless extensions (bug 564667), that probably won't include overlay support though.

Comment 14

[Atul Varma [:atul]]

Attachment: latest patch w/ Mossop's suggestions incorporated

Comment 15

[Atul Varma [:atul]]

Attachment: latest patch to work around bug 566485

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 446043 [details] [diff] [review]
latest patch to work around bug 566485

You could also just leave out the install/uninstall methods if you're never going to use them but I'm fine with this either way,

Comment 17

[Atul Varma [:atul]]

Hooray! Thanks.

I wanted to leave the install/uninstall empty for now, just b/c bootstrap.js is a new mechanism and I thought it'd be nice for the bootstrap.js file to be as documented as possible for now. I also suspect we will actually be using those methods really soon (for e.g. telling adw's simple-storage to delete its addon's db on uninstall).

Comment 18

[Atul Varma [:atul]]

Pushed: http://hg.mozilla.org/labs/jetpack-sdk/rev/d6ff967fb9d8

Comment 19

[Atul Varma [:atul]]

Also filed bug 566723 to make sure we revert the workaround I implemented in this patch once bug 566485 is fixed.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".