Closed Bug 1190995 Opened 5 years ago Closed 5 years ago

Support the new extension model in b2g

Categories

(Firefox OS Graveyard :: Runtime, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
FxOS-S5 (21Aug)
Tracking Status
firefox43 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(1 file, 2 obsolete files)

This bug covers the b2g specific changes needed to get the support added in bug 1175770 to work in b2g.
Depends on: 1125916
Attached patch wip (obsolete) — Splinter Review
So, that works, with extensions being simply added to app packages. We end up with both manifest.json (for extension) and our current manifest.webapp in the same zip.

I still need to fix the use of nsIAddonPolicyService and update tests.
Attachment #8643197 - Flags: review?(wmccloskey)
Attachment #8643197 - Flags: review?(wmccloskey)
Attached patch b2g-extensions.patch (obsolete) — Splinter Review
Now with @mozilla.org/addons/policy-service;1 packaged in b2g.
Attachment #8643197 - Attachment is obsolete: true
> So, that works, with extensions being simply added to app packages. We end up with both
> manifest.json (for extension) and our current manifest.webapp in the same zip.

Do you mean that the addon must be packaged with both manifests on the server side? Or does the server side package just include the manifest.json, and we generate the manifest.webapp clientside somehow?
This patch expects both manifests to be in the zip, yes. We could do with one single "super manifest" if needed but we first need to check that there are no conflicts in the two formats.
I'd really like to not require a manifest.webapp in the server-provided package. Though of course we can fix that as a separate bug.

One option would be to generate a manifest.webapp which contains only the properties needed by gaia, and make mozApp.manifest return that manifest.
Still using 2 manifests, but now tests are passing.

Bill, if you are not comfortable reviewing the dom/apps changes I can ask someone else. It's a big simplification from what we had, since now we just have to startup/shutdown addons when apps are install/uninstalled or enabled/disabled.
Attachment #8643204 - Attachment is obsolete: true
Attachment #8643402 - Flags: review?(wmccloskey)
Jonas, both manifest use the "permissions" property, with a different syntax. We'll have to figure out how to deal with that.
Comment on attachment 8643402 [details] [diff] [review]
b2g-extensions.patch

Review of attachment 8643402 [details] [diff] [review]:
-----------------------------------------------------------------

I think I understand the UserCustomizations.jsm changes enough to properly review.

::: b2g/app/b2g.js
@@ +1169,5 @@
>  pref("dom.presentation.device.name", "Firefox OS");
> +
> +// XXX : For testing purposes.
> +pref("dom.apps.developer_mode", true);
> +pref("network.disable.ipc.security", true);

This looks bad :-).

::: dom/apps/UserCustomizations.jsm
@@ -256,5 @@
> -
> -    // Makes sure we get rid of the sandbox.
> -    if (sandbox) {
> -      aWindow.addEventListener("unload", () => {
> -        Cu.nukeSandbox(sandbox);

Hmm, I never call nukeSandbox in the extension code. It seems like a good idea. Would you mind adding it here?
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#234
The sandbox will be in this.sandbox.

@@ -281,5 @@
> -
> -    this._items.forEach((aItem) => {
> -      // We only allow customizations to apply to apps with an equal or lower
> -      // privilege level.
> -      if (principal.appStatus > aItem.status) {

This definitely isn't something that's supported by the extension code. What does it do?

@@ +66,5 @@
>  
>    init: function() {
> +    // XXX : For testing purposes.
> +    Cu.import("resource://gre/modules/AppsUtils.jsm");
> +    AppsUtils.allowUnsignedAddons = true;

Not sure what this is for.

::: dom/browser-element/BrowserElementParent.js
@@ +234,5 @@
>                   .apply(self, arguments);
>        }
>      });
> +
> +    this._mm.loadFrameScript("chrome://global/content/extensions.js", true);

It seems a little odd to use a frame script. Don't we use BrowserElementChildPreload.js for everything else in b2g?

My one concern is that the code won't be loaded quickly enough when someone does window.open(). Maybe that's not an issue, though, since we're loading the content scripts asynchronously anyhow.

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp
@@ +307,5 @@
>    nsAutoCString scheme;
>    nsresult rv = baseURI->GetScheme(scheme);
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (!scheme.Equals(mScheme)) {
> +    if (mEnforceFileOrJar && !scheme.EqualsLiteral("file") && !scheme.EqualsLiteral("jar")

Maybe put each check on its own line so it's a little more symmetrical.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +151,5 @@
> +        // TODO: check if it's ok to use an async load here.
> +        let options = {
> +          target: sandbox,
> +          charset: "UTF-8",
> +          async: true

Can we set async to true only if |scheduled == "document_idle"|?

If we're on b2g, we really need to flag an error if someone uses anything besides "document_idle". Ideally that would happen when we validate the manifest before installation. But we don't validate the manifest yet :-). Maybe we could just use Cu.reportError here and ignore the script?
Attachment #8643402 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #8)
> Comment on attachment 8643402 [details] [diff] [review]
> b2g-extensions.patch
> 
> Review of attachment 8643402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I understand the UserCustomizations.jsm changes enough to properly
> review.
> 
> ::: b2g/app/b2g.js
> @@ +1169,5 @@
> >  pref("dom.presentation.device.name", "Firefox OS");
> > +
> > +// XXX : For testing purposes.
> > +pref("dom.apps.developer_mode", true);
> > +pref("network.disable.ipc.security", true);
> 
> This looks bad :-).

It is! That's why we depend on bug 1125916 and extensions only work in "developer mode" in b2g currently.

> Hmm, I never call nukeSandbox in the extension code. It seems like a good
> idea. Would you mind adding it here?
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionContent.jsm#234
> The sandbox will be in this.sandbox.

Will do.

> @@ -281,5 @@
> > -
> > -    this._items.forEach((aItem) => {
> > -      // We only allow customizations to apply to apps with an equal or lower
> > -      // privilege level.
> > -      if (principal.appStatus > aItem.status) {
> 
> This definitely isn't something that's supported by the extension code. What
> does it do?

That was preventing eg. a privileged extension to inject script in certified apps. But we don't need that with new security model.

> >    init: function() {
> > +    // XXX : For testing purposes.
> > +    Cu.import("resource://gre/modules/AppsUtils.jsm");
> > +    AppsUtils.allowUnsignedAddons = true;
> 
> Not sure what this is for.

Really, just to let me test by installing add-ons that are not signed. I will not land that.

> ::: dom/browser-element/BrowserElementParent.js
> @@ +234,5 @@
> >                   .apply(self, arguments);
> >        }
> >      });
> > +
> > +    this._mm.loadFrameScript("chrome://global/content/extensions.js", true);
> 
> It seems a little odd to use a frame script. Don't we use
> BrowserElementChildPreload.js for everything else in b2g?
> 
> My one concern is that the code won't be loaded quickly enough when someone
> does window.open(). Maybe that's not an issue, though, since we're loading
> the content scripts asynchronously anyhow.

Hm, I'll double check but I think I didn't get the right global in some cases when doing ExtensionContent.init(global) from BrowserElementChildPreload.j.

> ::: toolkit/components/extensions/ExtensionContent.jsm
> @@ +151,5 @@
> > +        // TODO: check if it's ok to use an async load here.
> > +        let options = {
> > +          target: sandbox,
> > +          charset: "UTF-8",
> > +          async: true
> 
> Can we set async to true only if |scheduled == "document_idle"|?
> 
> If we're on b2g, we really need to flag an error if someone uses anything
> besides "document_idle". Ideally that would happen when we validate the
> manifest before installation. But we don't validate the manifest yet :-).
> Maybe we could just use Cu.reportError here and ignore the script?

I'm fine with doing that for now, but that would be very sad to not support some extensions on b2g because of this async loading issue. I'll file a follow-up to come up with a better solution.
Blocks: 1191619
Blocks: addons25
https://hg.mozilla.org/mozilla-central/rev/fca7b12ccd91
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
You need to log in before you can comment on or make changes to this bug.