743359 - Land module loader to firefox

Status: RESOLVED FIXED

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

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

First thing we need to land an SDK loader to firefox is to implement support for a system modules described here: https://github.com/mozilla/addon-sdk/wiki/JEP-packageless

Goal of this task would be to make changes to a loader that would allow:

1. loading it as a JSM.
2. Instantiation of loader.
3. Use of loader instance to load system modules from `resouruces:///modules/` as described in the packageless proposal.

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

This would allow us to land in firefox and do the linker changes in parallel or independently.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

At the moment requirements of loader look as follows:


@packaging
api-utils/globals!
api-utils/process
api-utils/self!
api-utils/system
api-utils/unload

I think we should minimize that before attempts to land to firefox.

- @packaging is pseudo-module so it's fine

- api-utils/globals! This is layer of shims & workarounds for lack of system capabilities to write to stdout properly. I will try to separate it from loader.

- `api-utils/process` is there for E10S and in fact has lots of dependencies of it's
own so it would be good to get rid of it. @Mossop is there any news about E10S ? Maybe we should get rid our E10S abstraction layer it uses hidden window and bunch of other unnecessary stuff. 

- self! only loaded if require('self') is used maybe we could ignore it for the time being or update self so that it works the way it would in packageless.

- `api-utils/system` is there for a two reasons to provide backwards compatibility to
`exports.main` with `staticArgs` and `exit`.  

- `api-utils/unload` is there for a two reasons to provide a backwards compatibility for `exports.main` and to notify system about unload. The later one can be done using `nsIObserverService`.

I think we could make a module `deprecate/main` that would be loaded only if main.js exports `main` to provide these shims and recommend usage of `require('system')` instead.

Comment 4

[Alexandre Poirot [:ochameau]]

It looks like we should move Loader.main out of cuddlefish and move it to bootstrap.js or a module that will use cuddlefish in order to run an addon.

Comment 5

[Alexandre Poirot [:ochameau]]

If we get rid of E10S stuff (I agree we should remove it), here is what I would do:
https://github.com/ochameau/addon-sdk/compare/factors-out-addon-stuff-from-cuddlefish-without-e10s
This patch introduce a new module "chrome-entry-point", that factors out Loader.main and some stuff from bootstrap.js:shutdown that requires some module.
I think it would be helpfull to have such common js module to handle *Addon* loading/unloading. So that we can use other modules easily like l10n, unload or system for these actions.
What I like in this approach is that it allows to not change much stuff for jetpack, while removing dependencies on Cuddlefish!

We may even keep E10S support with similar pattern:
https://github.com/ochameau/addon-sdk/compare/master...factors-out-addon-stuff-from-cuddlefish
A similar patch that introduce 2 new modules instead of just 1.
- chrome-entry-point: creates the browser node and inject a loader in it, load the second module,
- remote-entry-point: load and run main addon module

These patches are somekind of draft. Names may be really bad. We may go futher and move more code in these modules. (Like setDefaultPrefs, startup listener,...)
I would be totally fine if you think it is a bad approach and would go in a completely different way too.

Comment 6

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1249d4fcdec82b4229a9563e3a6a1cf1d28e9421
Merge pull request #428 from Gozala/bug/loader-resolve@743359

Bug 743359 - Extract parts of loader that will land to firefox into separate module r=@ochameau

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4b6b60d818bc6c453d54c90f4a0c2721183634b7
Merge pull request #440 from Gozala/hotfix/loader-overlay@743359

Bug 743359 - Move module overlay logic from bootstrap to cuddlefish. r=@ochameau

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

After some discussing more details with @Mossop today we outlined couple of tasks that someone will need to implement:

1. I'm submitting patch against mozilla-central that @mossop is going to review.
2. We will need a makefile that will take care of copying modules from toolkit/addon-sdk/ to an appropriate place so that they appear under
resource://gre/modules/toolkit/. Same or similar makefile will copy files
from toolkit/addon-sdk/ to anappropriate place so that they appear under resource:///modules/ 
3. We will need a hook that will be invoked on commit to SDK's hg repo
and will copy SDK modules from to `toolkit/addon-sdk/`,  `browser/addon-sdk/` ..
depending on the module metadata.

Dietrich could someone from your team pick up 2-3 while ?

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Gavin, Dietrich told I should talk with you since have taken over his role now (Congrats BTW)

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/74513c83a89438b4bf18770530448fca5d3dbce3
Merge pull request #443 from Gozala/hotfix/loadReason@743359

Bug 743359 - Fixing issues introduced by #428 r=@ochameau

Comment 11

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

SecReview Scheduled:
https://mail.mozilla.com/home/[e-mail]/Security%20Review.html?view=month&action=view&invId=110490-110489&pstat=AC&exInvId=110490-180042&useInstance=1&instStartTime=1337965200000&instDuration=3600000

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/2

Pointer to Github pull-request

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I have updated document describing how synchronization of SDK repo
with mozilla-central should work with https://github.com/mozilla/addon-sdk/wiki/JEP-SDK-in-Firefox

Mossop: Could you please take a look to make sure I have not missed or miss-interpreted something.

Gavin: This should capture details for 2. 3. Let me know if you need
any more information to get someone started with these ?

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Created 2. and 3. tasks as blocking bug 756542 and bug 756545 for this one.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #8)
> 2. We will need a makefile that will take care of copying modules from
> toolkit/addon-sdk/ to an appropriate place so that they appear under
> resource://gre/modules/toolkit/. Same or similar makefile will copy files
> from toolkit/addon-sdk/ to anappropriate place so that they appear under
> resource:///modules/ 

I assume that second "toolkit/addon-sdk/" is meant to be "browser/addon-sdk"?

An initial patch that adds the current modules to those directories would be useful for testing, but the build goop to do this is relatively simple. We can discuss further in bug 756542.

> 3. We will need a hook that will be invoked on commit to SDK's hg repo
> and will copy SDK modules from to `toolkit/addon-sdk/`, 
> `browser/addon-sdk/` ..
> depending on the module metadata.

I don't know who the best person is to write an hg hook for the SDK repo. But is that really blocking this work? How often are new modules added? Can we not just sync them manually periodically?

Comment 16

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> I assume that second "toolkit/addon-sdk/" is meant to be "browser/addon-sdk"?

There is ongoing work to add support for Add-ons SDK to other applications like Thunderbird. So 'browser' doesn't seem to be the right place.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

There are going to be two module locations, one in browser/ and one in toolkit/. His comment just mistakenly referred to both as being "toolkit/addon-sdk" - that's why I said "that second".

Comment 18

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/448

Pointer to Github pull-request

Comment 19

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 626207 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/448

Ochameau or Mossop please pick up this review.

Comment 20

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #17)
> There are going to be two module locations, one in browser/ and one in
> toolkit/. His comment just mistakenly referred to both as being
> "toolkit/addon-sdk" - that's why I said "that second".

Yeah you're right, sorry for that.

Comment 21

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 626207 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/448

I forgot I have created a Bug 756548 for this one, moving pathch there.

Comment 22

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/3

Pointer to Github pull-request

Comment 23

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/302a5533e975f73a987450ee73cfd948b9694aff
Fix regression from bug 743359. logFile and resultFile are always set but may be empty.

https://github.com/mozilla/addon-sdk/commit/490f59c8a568d765eb55022eb391a405d7a8c40c
Merge pull request #460 from ochameau/fix/loader-regression

Fix regression from bug 743359. r=@gozala

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 629251 [details]
Pointer to Github pull request: https://github.com/mozilla/mozilla-central/pull/3

I think we've iterated over all this enough elsewhere, this looks good to me.

Comment 25

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Dave Townsend (:Mossop) from comment #24)
> Comment on attachment 629251 [details]
> Pointer to Github pull request:
> https://github.com/mozilla/mozilla-central/pull/3
> 
> I think we've iterated over all this enough elsewhere, this looks good to me.

In that case would you be that kind to land this change ?

Thanks!

Comment 26

[Dave Townsend [:mossop]]

Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/efa8bb276e3d

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/efa8bb276e3d