Closed Bug 532724 Opened 15 years ago Closed 15 years ago

can't modify settings if you import them before declaring a manifest

Categories

(Mozilla Labs :: Jetpack Prototype, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

Details

Attachments

(1 file, 1 obsolete file)

If you import settings before declaring a manifest containing settings, you won't be able to modify the settings afterwards.

We have two options to fix this:

1. Lazily initialize settings when first accessed rather than when imported.
2. Require that the manifest be declared before settings are imported (and document this fact in the JEP and documentation).

I'm leaning towards the latter, on the basis that manifests in Jetpacks should work the same whether they are inline or in a separate file (which will be the case once we enable jetpacks to be packaged); in particular, that they should be considered static information that is not modifiable at runtime.

However, I'm open to alternative opinions...

In any case, we should figure this out for 0.7 if possible.  Setting target milestone accordingly.
Comment on attachment 415951 [details] [diff] [review]
patch v1: requires that the manifest be declared before importing settings

>+++ b/extension/content/js/jetpack-runtime.js
>@@ -83,18 +83,28 @@ var JetpackRuntime = {
>     this.__defineGetter__(
>       "manifest",
>       function() {
>         // TODO: Ultimately we should get the manifest via static
>         // analysis of the code, or by executing the Jetpack in an
>         // extremely limited sandbox.
>-        if ("manifest" in this.unsafeSandbox)
>+        if ("manifest" in this.unsafeSandbox) {
>+          if (typeof this.unsafeSandbox.manifest != "object") {
>+            // XXX Should this throw instead of simply reporting an error?
>+            Cu.reportError("<" + this.srcUrl + ">: manifest is not object");
>+            return null;
>+          }
This "object" check seems unnecessary. As long as it's not null-ish, it can have properties accessed.

>+        // XXX Should we construct an empty manifest for jetpacks that don't
>+        // provide one (or whose manifests are not objects)?  That way consumers
>+        // wouldn't have to always check for the existence of a manifest; they
>+        // would be able to simply check if it has the property they care about.
>         return null;
This seems reasonable. So combining these two, we could just do..

let manifest = this.unsafeSandbox.manifest;
return manifest == null ? {} : manifest;

(Or probably equally sane.. return this.unsafeSandbox.manifest || {}; The only thing we lose out on manifests of 0, false, "".)

Unrelated but touching this code.. If we assume the manifests are unmodifiable, the getter should probably replace itself with the first value it finds instead of always referring to the unsafeSandbox, which potentially could be changed. (But even then, if the jetpack feature modifies manifest.settings = {} instead of doing manifest = { settings: {} }, it would still seem like settings could be changed after the fact...)

>+++ b/extension/content/js/settings-store.js
>@@ -39,24 +39,23 @@ let HOSTNAME = "chrome://jetpack";
>+  if (!("manifest" in context) || !context.manifest)
>+    throw("can't initialize settings: no manifest");
When wouldn't there be a manifest in context? Also with the fix above, no need to check for manifest missing.
(In reply to comment #2)
> This "object" check seems unnecessary. As long as it's not null-ish, it can
> have properties accessed.

Yup, good point.


> This seems reasonable. So combining these two, we could just do..
> 
> let manifest = this.unsafeSandbox.manifest;
> return manifest == null ? {} : manifest;
> 
> (Or probably equally sane.. return this.unsafeSandbox.manifest || {}; The only
> thing we lose out on manifests of 0, false, "".)

Makes sense, that's what I've done.


> Unrelated but touching this code.. If we assume the manifests are unmodifiable,
> the getter should probably replace itself with the first value it finds instead
> of always referring to the unsafeSandbox, which potentially could be changed.

Yeah, I've done that, despite its limitations (we should probably come up with a better solution in the long run, perhaps ES2 object locking?).


> >+++ b/extension/content/js/settings-store.js
> >@@ -39,24 +39,23 @@ let HOSTNAME = "chrome://jetpack";
> >+  if (!("manifest" in context) || !context.manifest)
> >+    throw("can't initialize settings: no manifest");
> When wouldn't there be a manifest in context? Also with the fix above, no need
> to check for manifest missing.

Good points, check removed.
Attachment #415951 - Attachment is obsolete: true
Attachment #418057 - Flags: review?(edilee)
Attachment #415951 - Flags: review?(avarma)
Attachment #418057 - Flags: review?(edilee) → review+
Fixed by changeset http://hg.mozilla.org/labs/jetpack/rev/7b24d5e567a8.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: