Closed Bug 1333477 Opened 9 years ago Closed 8 years ago

Support injecting and invalidating schema wrappers dynamically

Categories

(WebExtensions :: General, defect)

Product:
WebExtensions
Component:
General
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: aswan, Assigned: kmag)

References

Details

(Whiteboard: triaged)

Attachments

(3 files)

Bug 1333477: Part 1 - Bail out earlier for unsupported schema entries.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1333477: Part 2 - Remove "browser" environment from ESLint settings.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Our schema wrappers are currently generated and injected into appropriate contexts based on fixed properties in an extension's manifest. With optional permissions, individual permissions can be added and removed dynamically at runtime. So we need a way to inject and invalidate corresponding schema wrappers dynamically. Note that just removing schema wrappers from the chrome/browser namespaces when a permission is revoked is not sufficient since an extension could keep a reference around. This is just a building block for optional permissions, things like events will require separate handling and are outside of the scope of this bug.
Whiteboard: triaged
Depends on: 1338409
Comment on attachment 8843642 [details] Bug 1333477: Part 1 - Bail out earlier for unsupported schema entries. https://reviewboard.mozilla.org/r/117270/#review119354
Attachment #8843642 - Flags: review?(aswan) → review+
Comment on attachment 8843643 [details] Bug 1333477: Part 2 - Remove "browser" environment from ESLint settings. https://reviewboard.mozilla.org/r/117272/#review119366 ::: toolkit/components/extensions/test/mochitest/.eslintrc.js:7 (Diff revision 1) > > module.exports = { // eslint-disable-line no-undef > "extends": "../../../../../testing/mochitest/mochitest.eslintrc.js", > > "env": { > + "browser": true, why is this needed? mochitest.eslintrc.js already specifies it ::: toolkit/components/extensions/test/xpcshell/.eslintrc.js:10 (Diff revision 1) > + "env": { > + "browser": true, > + } shouldn't this go in /testing/xpcshell/xpcshell.eslintrc.js ?
Attachment #8843643 - Flags: review?(aswan) → review+
Comment on attachment 8843643 [details] Bug 1333477: Part 2 - Remove "browser" environment from ESLint settings. https://reviewboard.mozilla.org/r/117272/#review119366 > why is this needed? mochitest.eslintrc.js already specifies it Our override in the parent manifest takes precedence. > shouldn't this go in /testing/xpcshell/xpcshell.eslintrc.js ? No, in general we don't want xpcshell tests to have the browser environment, but since most of our tests have things like background scripts that do run in a browser environment, turning it off is extremely inconvenient.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119372 I haven't fully grokked all of this, need to take another pass but here are a few questions that came up on the first pass ::: toolkit/components/extensions/Schemas.jsm:492 (Diff revision 1) > + this.context = context; > + this.path = path; > + this.parentObj = parentObj; > + this.name = name; > + this.entry = entry; > + this.parentEntry = parentEntry; nit: can you order these the same as the parameters? ::: toolkit/components/extensions/Schemas.jsm:561 (Diff revision 1) > + } catch (e) { > + Cu.reportError(e); > + } in what circumstances does this fail? ::: toolkit/components/extensions/Schemas.jsm:662 (Diff revision 1) > */ > class InjectionContext extends Context { > constructor(params) { > super(params, CONTEXT_FOR_INJECTION); > + > + this.pendingEntries = new Set(); Do I understand correctly that this is the collection of all entries that have been inserted dynamically? If so, I think the name is confusing, maybe `dynamicEntries` ?
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119372 > in what circumstances does this fail? It can fail if content code redifined the property as non-configurable. That's unlikely to actually happen in practice, but I'd still rather we not fail catastrophically if it does. Should really probably just use Reflect.deleteProperty instead. > Do I understand correctly that this is the collection of all entries that have been inserted dynamically? If so, I think the name is confusing, maybe `dynamicEntries` ? All entries that have been inserted dynamically, or have not been inserted, but could be inserted dynamically.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119372 > All entries that have been inserted dynamically, or have not been inserted, but could be inserted dynamically. so can we call it `revokableEntries` or something? sorry to nitpick but pending really doesn't sound descriptive to me.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119372 > so can we call it `revokableEntries` or something? sorry to nitpick but pending really doesn't sound descriptive to me. Well, they're either pending injection or pending revocation. I think `revokableEntries` implies that they've all already been injected, which isn't the case.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119434 Still working on this, a few more comments ::: toolkit/components/extensions/Schemas.jsm:587 (Diff revision 1) > + if (this.isRevokable) { > + this.context.pendingEntries.add(this); > + } nit: can we do this from `lazyInject()`? getters with side effects make me antsy. ::: toolkit/components/extensions/Schemas.jsm:609 (Diff revision 1) > + if (this._neuter) { > + this._neuter(); > + } I don't get this, this appears to only be invoked if this is called twice. Can we just check `this.injected` and bail out here if this has already been injected? Is the point that we want to force the permission check to happen again when this is called a second time? If that's the case, I'm confused why the regular revocation code doesn't just handle that... ::: toolkit/components/extensions/Schemas.jsm:641 (Diff revision 1) > + this.maybeInject(); > + } > + } > + > + maybeInject() { > + if (!this.injected && !this._neuter) { The only place this is called is from a few lines above where its is only called if `!this.injected`, making the first clause here irrelevant. And I can't think of cases where we might expect to add other calls to this that need the extra check... ::: toolkit/components/extensions/Schemas.jsm:647 (Diff revision 1) > + this.lazyInject(); > + } > + } > + > + maybeRevoke() { > + if (this.injected && !this.hasPermission) { ditto the above comment
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119434 > I don't get this, this appears to only be invoked if this is called twice. Can we just check `this.injected` and bail out here if this has already been injected? Is the point that we want to force the permission check to happen again when this is called a second time? If that's the case, I'm confused why the regular revocation code doesn't just handle that... There are two cases we need to worry about here: 1) The entry has already been injected, and the getter is called again. This handled for us by the `exportLazyProperty` helper. 2) The getter has never been called, but the content has stored it. After we revoke the entry, they could call that lazy getter and cause us to inject the value, leaving us in an inconsistent state. The neuter check handles the second issue.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119434 > nit: can we do this from `lazyInject()`? getters with side effects make me antsy. No, there are other places this is called from, and we need to do this in those cases, too.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119434 > There are two cases we need to worry about here: > > 1) The entry has already been injected, and the getter is called again. This handled for us by the `exportLazyProperty` helper. > 2) The getter has never been called, but the content has stored it. After we revoke the entry, they could call that lazy getter and cause us to inject the value, leaving us in an inconsistent state. > > The neuter check handles the second issue. Okay, that's obscure enough that I think a comment explaining that scenario would be good.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119434 > Okay, that's obscure enough that I think a comment explaining that scenario would be good. Wait upon further reading, if they store the getter and then call it after the permission has been revoked, the getter calls `getDescriptor()` which does a permission check. If I've got this right, at that point the permission check fails, the getter returns undefined, and `exportLazyProperty()` does the right thing. What am I missing?
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119478 ::: toolkit/components/extensions/Schemas.jsm:559 (Diff revision 1) > + if (this.injected.revoke) { > + this.injected.revoke(); > + } > + > + try { > + let unwrapped = Cu.waiveXrays(this.parentObj); nit: does this line need to be inside the try block? ::: toolkit/components/extensions/Schemas.jsm:595 (Diff revision 1) > + > + if (!this.hasPermission) { > + return; > + } > + > + this.injected = this.entry.getDescriptor(this.path, this.context); Can we get here when `this.injected` is already set? If so, the old one will be orphaned an un-revokable. Can we assert that it isn't set (or if it is, just return its existing descriptor)? ::: toolkit/components/extensions/Schemas.jsm:609 (Diff revision 1) > + * Injects a lazy property descriptor into the parent object which > + * checks permissions and eligibility for injection the first time it > + * is accessed. > + */ > + lazyInject() { > + if (this._neuter) { Maybe I just haven't yet understood the case where this is necessary but I don't see it yet and I think this whole thing would be simpler and hence more maintainable if we could get rid of `_neuter` ::: toolkit/components/extensions/Schemas.jsm:632 (Diff revision 1) > + permissionsChanged() { > + if (this.injected) { > + this.maybeRevoke(); > + } else { > + this.maybeInject(); > + } > + } > + > + maybeInject() { > + if (!this.injected && !this._neuter) { > + this.lazyInject(); > + } > + } > + > + maybeRevoke() { > + if (this.injected && !this.hasPermission) { > + this.revoke(); > + } since maybeInject and maybeRevoke are only called from permissionsChanged, how about just inlining them? ::: toolkit/components/extensions/Schemas.jsm:762 (Diff revision 1) > + * The full path from the root injection object to this entry. > + * @param {Entry} parentEntry > + * The parent entry for this entry. > + * > + * @returns {object?} > + * A property descriptor object, or null if the entry should nit: i think this actually returned undefined (not null) if there's nothing to inject
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119478 > nit: does this line need to be inside the try block? For the sake of variable lifetimes, yes. > Can we get here when `this.injected` is already set? If so, the old one will be orphaned an un-revokable. Can we assert that it isn't set (or if it is, just return its existing descriptor)? In practice, we shouldn't, but I can add an assertion. > Maybe I just haven't yet understood the case where this is necessary but I don't see it yet and I think this whole thing would be simpler and hence more maintainable if we could get rid of `_neuter` It would be more simple but less correct. > since maybeInject and maybeRevoke are only called from permissionsChanged, how about just inlining them? I could, but I wrote these before I wrote `permissionChanged`, and I found it more readable this way than with them inlined. I don't have a particularly strong opinion on it, though.
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review119478 > It would be more simple but less correct. That's not a helpful reply. Here's where I'm confused: `_neuter` just causes the getter to return immediately. But the getter does an early permission check and doesn't return a usable descriptor if the permission has been revoked when it is called. So is the concern the case that you get two different getters and they both get called and so one of them becomes unrevokable? That sounds to be like the scenario covered in the comment right above this one?
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review120176 Maybe it just all clicked on the Nth reading but with the recent changes I do find this more understandable. And cheers for InjectionEntry cleaning up all the repeated allowedContexts code. I haven't yet worked through connecting this with bug 1197420 but it seems like it all ought to work. Thanks! ::: toolkit/components/extensions/Schemas.jsm:500 (Diff revision 2) > + this.name = name; > + this.entry = entry; > + this.parentEntry = parentEntry; > + this.injected = null; > + > + this._neuter = null; i think this can go now? or be replaced by an initial value for `lazyInjected` ::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_revoke.js:441 (Diff revision 2) > + equal(root.revokableNs, undefined, "Namespace is not defined"); > + checkCaptured(); > +}); > + > + > +add_task(function* test_neuter() { nit: this might be more aptly described as something like `test_lazy_getter` or something, and a comment explaining the scenario it is testing would be nice.
Attachment #8843644 - Flags: review?(aswan) → review+
Comment on attachment 8843644 [details] Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. https://reviewboard.mozilla.org/r/117274/#review120176 I was thinking we'd do something like emit an event on the Extension whenever its permissions change, and then have each context listen for those events, and call the permissions changed callbacks whenever it receives them. Probably worth nothing that we may wind up with multiple permissions changed callbacks for each context, since we inject the `chrome` and `browser` namespaces separately. > i think this can go now? or be replaced by an initial value for `lazyInjected` Oops...
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ba8ff8e7b4074d9328fed9f78ce981d817e4a7 Bug 1333477: Part 1 - Bail out earlier for unsupported schema entries. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/12dc344c2cdf5498e36cf2c819d6497590e83239 Bug 1333477: Part 2 - Remove "browser" environment from ESLint settings. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/3fd634d88e65bfffc260274da94694b9c74e1e0b Bug 1333477: Part 3 - Support dynamically injecting and revoking schema properties based on permissions. r=aswan
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: