1287010 - Some API methods need to run locally with OOP webextensions

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Description

•

8 years ago

Some API methods should run in the extension process rather than the main process. Examples: chrome.extension.getViews chrome.extension.getBackgroundPage chrome.extension.getURL chrome.extension.getExtensionTabs chrome.runtime.getManifest Generally anything that returns data directly rather than via a callback needs to be handled in this way. I think we'll need to update our schemas to say where the code for an API should run and then have some mechanism for intercepting these special calls in the extension process.

:shell escalante

Updated

•

8 years ago

Whiteboard: triaged

Rob Wu [:robwu]

Assignee

Comment 1

•

8 years ago

Taking this. This blocks bug 1287007, because in order to move APIs over to childmanager, the implementation has to be split in two parts (one that always runs locally, and one that may run remotely).

Assignee: nobody → rob

Blocks: 1287007

Status: NEW → ASSIGNED

No longer depends on: 1287007

Rob Wu [:robwu]

Assignee

Updated

•

8 years ago

Blocks: 1286712

Comment hidden (mozreview-request)

Matthew Wein [:mattw]

Updated

•

8 years ago

Blocks: 1295807

Comment hidden (mozreview-request)

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 20

•

8 years ago

mozreview-review

Comment on attachment 8783245 [details] Bug 1287010 - Use minimal global scope for ext-*.js scripts https://reviewboard.mozilla.org/r/73152/#review71200 ::: toolkit/components/extensions/ExtensionGlobalScope.jsm:31 (Diff revision 2) > +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionUtils", > + "resource://gre/modules/ExtensionUtils.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "require", > + "resource://devtools/shared/Loader.jsm"); > + > +let _internalGlobals = new WeakMap(); Please don't use underscore prefixes. If it's not exported, it should be considered private (even though there's a way around that, as you've seen). ::: toolkit/components/extensions/ExtensionGlobalScope.jsm:55 (Diff revision 2) > + */ > +this.loadExtScriptInScope = (scriptUrl, apiManager) => { > + // Different SchemaAPIManagers should have different globals. > + let global = _internalGlobals.get(apiManager); > + if (!global) { > + global = Object.create(this); Can we make the proto be null here? Anything in ExtensionGlobalScope will still be accessible since it's the global.

Attachment #8783245 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 21

•

8 years ago

Hi Rob, Can you post a single patch that includes all the changes? It's great that you've managed to split it up like this, but I'm having a hard time reviewing it this way. It would be much easier if I could see the final version of everything. I'm not sure if you can do this in ReviewBoard. If not, can you just post a normal Splinter patch? I'm pretty close to understanding it all, but I still want to make one final pass over everything. Thanks, Bill

Rob Wu [:robwu]

Assignee

Comment 22

•

8 years ago

mozreview-review-reply

Comment on attachment 8783245 [details] Bug 1287010 - Use minimal global scope for ext-*.js scripts https://reviewboard.mozilla.org/r/73152/#review71200 > Please don't use underscore prefixes. If it's not exported, it should be considered private (even though there's a way around that, as you've seen). The scripts run in this global scope and can therefore make use of any variables declared here. With the guarantee that these scripts can use the globals, the Cu/Ci/Cc/XPComUtils boilerplate can be removed from the scripts (many scripts already assume that these exist). I've added an underscore to make sure that it is really private, even though the global namespace is shared with the other ext-*.js scripts. > Can we make the proto be null here? Anything in ExtensionGlobalScope will still be accessible since it's the global. I'll try it (on try).

Rob Wu [:robwu]

Assignee

Comment 23

•

8 years ago

(In reply to Bill McCloskey (:billm) from comment #21) > Hi Rob, > Can you post a single patch that includes all the changes? It's great that > you've managed to split it up like this, but I'm having a hard time > reviewing it this way. It would be much easier if I could see the final > version of everything. I'm not sure if you can do this in ReviewBoard. If > not, can you just post a normal Splinter patch? > > I'm pretty close to understanding it all, but I still want to make one final > pass over everything. Click on the "Squashed Diff" link at the top of reviewboard.

Comment hidden (mozreview-request)

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 35

•

8 years ago

mozreview-review

Comment on attachment 8784270 [details] Bug 1287010 - Use sandbox instead of JSM for global separation https://reviewboard.mozilla.org/r/73786/#review71796 ::: toolkit/components/extensions/ExtensionUtils.jsm:1681 (Diff revision 1) > + * @param {string} scriptUrl The URL of the ext-*.js script. > + */ > loadScript(scriptUrl) { > - loadExtScriptInScope(scriptUrl, this); > + // Create the object in the context of the sandbox so that the script runs > + // in the sandbox's context instead of here. > + let scope = this.global.Object.create(null); I don't think this will create the object in the correct global. You should use Cu.createObjectIn.

Attachment #8784270 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 36

•

8 years ago

mozreview-review

Comment on attachment 8784271 [details] Bug 1287010 - Refactor shouldInject / pathObj https://reviewboard.mozilla.org/r/73788/#review71808 ::: toolkit/components/extensions/Extension.jsm:296 (Diff revision 1) > - for (let elt of path) { > // If we get a null object before reaching the requested path > // (e.g. the API object is returned only on particular kind of contexts instead > // of based on WebExtensions permissions, like it happens for the devtools APIs), > // stop searching and return undefined. > + // TODO(robwu): This should never be reached. If an API is not available for Why is this a TODO? ::: toolkit/components/extensions/ExtensionUtils.jsm:1425 (Diff revision 1) > + * An object that runs the implementation of a schema API. Instantiations of > + * this interfaces are used by Schemas.jsm. > + * > + * @interface > + */ > +class ISchemaAPIImplementation { // eslint-disable-line no-unused-vars Please just call this SchemaAPIInterface. ::: toolkit/components/extensions/ExtensionUtils.jsm:1657 (Diff revision 1) > + return; > + } > + set.remove(listener); > + > + if (set.size == 0) { > + this.childApiManager.messageManager.sendAsyncMessage("Extension:RemoveListener", { This seems to be a pre-existing bug. Should be API:RemoveListener. ::: toolkit/components/extensions/Schemas.jsm:1269 (Diff revision 1) > if (this.hasAsyncCallback) { > callback = actuals.pop(); > } > - return context.callAsyncFunction(pathObj, path, name, actuals, callback); > + if (callback === null && context.isChromeCompat) { > + // We pass an empty stub function as a default callback for > + // the `chrome` API, so promisr objects are not returned, promise, not promisr

Attachment #8784271 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 37

•

8 years ago

mozreview-review

Comment on attachment 8783246 [details] Bug 1287010 - Add pathObj parameter to Schemas https://reviewboard.mozilla.org/r/73154/#review71218

Attachment #8783246 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 38

•

8 years ago

mozreview-review

Comment on attachment 8783247 [details] Bug 1287010 - Make environment of Context explicit. https://reviewboard.mozilla.org/r/73156/#review71214 ::: toolkit/components/extensions/Extension.jsm:166 (Diff revision 2) > - * Used to determine whether the API should be generated when the caller > - * requests a subset of the available APIs (e.g. in content scripts). > + * Intended to match the namespace of the generated API, but not used at > + * the moment - see bugzil.la/1295774. > + * @param {string} envType Restricts the API to contexts that run in the > + * given environment. Must be one of the following: > + * - "addon_parent" - addon APIs that runs in the main process. > + * - "addon_child" - addon APIs that may be run in an addon process. Can we take out "may be"?

Attachment #8783247 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 39

•

8 years ago

mozreview-review

Comment on attachment 8783248 [details] Bug 1287010 - Move Management logic to SchemaAPIManager https://reviewboard.mozilla.org/r/73158/#review71224

Attachment #8783248 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 40

•

8 years ago

mozreview-review

Comment on attachment 8783249 [details] Bug 1287010 - Prepare for moving content script APIs to schemas https://reviewboard.mozilla.org/r/73160/#review71256 ::: toolkit/components/extensions/ExtensionUtils.jsm:1434 (Diff revision 2) > - constructor(context, messageManager, namespaces, contextData) { > + constructor(context, messageManager, localApi, contextData) { > this.context = context; > this.messageManager = messageManager; > - this.namespaces = namespaces; > > + this.localApi = localApi; Let's call this localApis. Also, please comment what this is for.

Attachment #8783249 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 41

•

8 years ago

mozreview-review

Comment on attachment 8783250 [details] Bug 1287010 - Use schema-generated i18n for all contexts https://reviewboard.mozilla.org/r/73162/#review71832

Attachment #8783250 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 42

•

8 years ago

mozreview-review

Comment on attachment 8783251 [details] Bug 1287010, 1286712 - Use schema-generated runtime API, split ext-runtime.js https://reviewboard.mozilla.org/r/73164/#review71834

Attachment #8783251 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 43

•

8 years ago

mozreview-review

Comment on attachment 8783252 [details] Bug 1287010 - Use schema-generated extension, split ext-extension.js https://reviewboard.mozilla.org/r/73166/#review71836

Attachment #8783252 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 44

•

8 years ago

mozreview-review

Comment on attachment 8783253 [details] Bug 1287010 - Extension.jsm optimization: Use pathObj https://reviewboard.mozilla.org/r/73168/#review71838

Attachment #8783253 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 45

•

8 years ago

Also, as we discussed, I would like the SchemaAPIImplementation stuff to inherit from SchemaAPIInterface.

Rob Wu [:robwu]

Assignee

Comment 46

•

8 years ago

mozreview-review-reply

Comment on attachment 8784271 [details] Bug 1287010 - Refactor shouldInject / pathObj https://reviewboard.mozilla.org/r/73788/#review71808 > Why is this a TODO? Because this bail-out only works for local APIs, and not for remote APIs. Local APIs would be excluded through shouldInject, but with remote APIs the schema would generate the API which is proxied to a dead end. I've expanded on the comment.

Comment hidden (mozreview-request)

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 58

•

8 years ago

mozreview-review

Comment on attachment 8784610 [details] Bug 1287010 - Add test for SchemaAPIManager's loadScript https://reviewboard.mozilla.org/r/73968/#review71862

Attachment #8784610 - Flags: review?(wmccloskey) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

Reporter

Comment 59

•

8 years ago

mozreview-review

Comment on attachment 8784611 [details] Bug 1287010 - s/Extension:RemoveListener/API:RemoveListener/ https://reviewboard.mozilla.org/r/73970/#review71864

Attachment #8784611 - Flags: review?(wmccloskey) → review+

Comment hidden (mozreview-request)

Rob Wu [:robwu]

Assignee

Comment 73

•

8 years ago

The try server shows many red entries because some jobs have no WebExtensions test in their set, and "--tag webextensions" filters out those.

Keywords: checkin-needed

Pulsebot

Comment 74

•

8 years ago

Keywords: checkin-needed

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 75

•

8 years ago

bugherder

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox51: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla51

BMO Automation

Updated

•

6 years ago

Product: Toolkit → WebExtensions

Bug 1287010 - Use minimal global scope for ext-*.js scripts 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Add pathObj parameter to Schemas 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Make environment of Context explicit. 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Move Management logic to SchemaAPIManager 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Prepare for moving content script APIs to schemas 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Use schema-generated i18n for all contexts 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010, 1286712 - Use schema-generated runtime API, split ext-runtime.js 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Use schema-generated extension, split ext-extension.js 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Extension.jsm optimization: Use pathObj 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Use sandbox instead of JSM for global separation 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Refactor shouldInject / pathObj 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - Add test for SchemaAPIManager's loadScript 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details
Bug 1287010 - s/Extension:RemoveListener/API:RemoveListener/ 8 years ago Rob Wu [:robwu] (deleted), text/x-review-board-request	billm : review+	Details