Closed Bug 1287007 Opened 3 years ago Closed 3 years ago

Start using ChildAPIManager for some APIs

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: billm, Assigned: robwu)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(41 files, 2 obsolete files)

58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
bsilverberg
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
rhelmer
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
kmag
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
I think this is probably the first step for bug 1190679. We'll start transitioning individual APIs over to the ChildAPIManager. We'll be modifying the code at [1] to do this. We'll need to choose a message manager based on the context we're injecting into I guess. As a hack, we could use Services.cpmm and change ParentAPIManager to listen on Services.ppmm as well as Services.mm. Longer term, we'll need to use frame scripts for loading the APIs. That could cause some tricky issues with asynchrony though.

[1] http://searchfox.org/mozilla-central/rev/e269c68bc594b4a858624d73f0d9eb002f3c0844/toolkit/components/extensions/Extension.jsm#526
Whiteboard: triaged
Assignee: nobody → rob
No longer blocks: 1287010
Depends on: 1287010
Blocks: 1286712
Comment on attachment 8782677 [details]
Bug 1287007 - Use minimal global scope for ext- scripts

Patch was associated with the wrong bug.
Attachment #8782677 - Attachment is obsolete: true
Attachment #8782677 - Flags: review?(wmccloskey)
No longer blocks: 1286712
Depends on: 1296896
Depends on: 1298979
Depends on: 1300234
Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

https://reviewboard.mozilla.org/r/77086/#review75642

::: toolkit/components/extensions/ExtensionUtils.jsm:1749
(Diff revision 1)
> -          deferred.resolve(new SpreadArgs(data.args));
> +          let {args} = data;
> +          deferred.resolve(args.length == 1 ? args[0] : new SpreadArgs(args));

I don't think this is the right place to fix this. I suspect the real problem is here:
http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/toolkit/components/extensions/ExtensionUtils.jsm#441-443

Note that the code when callback is non-null deals with SpreadArgs specially, but the null case doesn't. I think you probably want to move the args.length == 1 ? ... : ... stuff there instead.
Attachment #8788662 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

https://reviewboard.mozilla.org/r/77054/#review75652

::: toolkit/components/extensions/Extension.jsm:374
(Diff revision 1)
>      super.unload();
>      Management.emit("proxy-context-unload", this);
>    }
>  }
>  
> -function findPathInObject(obj, path) {
> +function findPathInObject(obj, path, isLookupOnly) {

Maybe change the name of the parameter to generateErrors and make the default value be true.

::: toolkit/components/extensions/Extension.jsm:391
(Diff revision 1)
>      // are excluded in `shouldInject` by this check, but remote APIs do not have
>      // this information and will therefore cause the schema API generator to
>      // create an API that proxies to a non-existing API implementation.
>      if (!obj || !(elt in obj)) {
> +      if (!isLookupOnly) {
> +        Cu.reportError(`findPathInObject: No obj found for path: ${path}`);

This is not a great message for add-on developers either. How about `API ${path} not found (it may be unimplemented by Firefox).`?
Attachment #8788663 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

https://reviewboard.mozilla.org/r/77056/#review75656

Thinking this over, I think it's a bad idea to make this code more complex. The only places I see where we use "async":true are in page_action.json. Could we just add a callback argument to these functions in the schema to fix this? I don't think that would be a problem.
Attachment #8788664 - Flags: review?(wmccloskey) → review-
Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

https://reviewboard.mozilla.org/r/77056/#review75656

Sounds good to me. It is incompatible with Chrome, but not that bad because code in Chrome works in Firefox. And these methods should be having async callbacks - https://crbug.com/451320.
Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

https://reviewboard.mozilla.org/r/77058/#review75764

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_api_injection.js:67
(Diff revision 1)
>    let url = "data:," + JSON.stringify(nestedNamespaceJson);
>  
>    // Load the schema of the fake APIs.
>    yield Schemas.load(url);
>  
> -  // Register an API that will skip the background page.
> +  let apiManager = new SchemaAPIManager("addon_child");

Should just be "addon" I think.
Attachment #8788665 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

https://reviewboard.mozilla.org/r/77060/#review75766

Everywhere it says "browserAction ready", it should instead say "pageAction ready"
Attachment #8788666 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

https://reviewboard.mozilla.org/r/77062/#review75768

These Promise.resolve() calls are not needed since that's the default. Also, as I said before, I would rather add a real callback param to these methods.
Attachment #8788667 - Flags: review?(wmccloskey) → review-
Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

https://reviewboard.mozilla.org/r/77064/#review75770
Attachment #8788668 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

https://reviewboard.mozilla.org/r/77066/#review75772

The sanitize code looks pretty dangerous to me. You're accessing properties on a waived object. Can't that invoke getters that will call arbitrary code? See extractProperties in Schemas.jsm for an example of how to do this more safely. But do we even need to do it? Functions shouldn't be passed in anyway, and the toJSON/toString doesn't necessarily seem like undesirable behavior either.

The other parts of the patch look fine.
Attachment #8788669 - Flags: review?(wmccloskey)
Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

https://reviewboard.mozilla.org/r/77068/#review75774

Please update the comment for ExtensionContext. It talks about running in the chrome process. Also, remove the mention of "extension page", since that concept was removed a while ago.
Attachment #8788670 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

https://reviewboard.mozilla.org/r/77070/#review75776
Attachment #8788671 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

https://reviewboard.mozilla.org/r/77072/#review75778

The windowId and tabId stuff isn't really correct. When a tab moves from one window to another, it gets a different <browser> element. So you can't save them like this.
However, these IDs should not change once the context is created. Could you save them initially?

However, it looks like these properties aren't used in this patch, so r+.

This also makes me realize that saving the messageManager in the context is incorrect (which is my fault, not yours). When a tab is moved to another window, the message manager changes. We could listen for events to swap the message managers accordingly, or we could eliminate our need for this property somehow. It's mostly not used so far.
Attachment #8788672 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

https://reviewboard.mozilla.org/r/77074/#review75780
Attachment #8788673 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

https://reviewboard.mozilla.org/r/77076/#review75782

::: toolkit/components/extensions/ext-c-backgroundPage.js:40
(Diff revision 1)
> +      getBackgroundPage,
> +    },
> +
> +    runtime: {
> +      getBackgroundPage() {
> +        return Promise.resolve(getBackgroundPage());

The change to runtime.getBackgroundPage doesn't seem safe to me. Won't this try to clone a window across scopes? That shouldn't work. Otherwise this patch looks fine.
Attachment #8788674 - Flags: review?(wmccloskey) → review+
This is as far as I got on the plane. I'll get to the rest as soon as I can.
Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

https://reviewboard.mozilla.org/r/77062/#review75768

Done (removed Promise.resolve()) and added a named callback to the schema).
Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

https://reviewboard.mozilla.org/r/77066/#review75772

The waiving and unwaving is indeed unnecessary. I started with turning functions into plain objects because Chrome does it as well. But then I saw that the unit test actually expected `undefined` and so I switched to that and did not remove the waiving.

Without the updated sanitizer, I would get a test failure/timeout with the "Permission denied to pass object to privileged code" error when I run toolkit/components/extensions/test/mochitest/test_ext_storage_content.html. The fact that toJSON, toString, etc. are no longer treated specially is a nice bonus (=better compatibility with Chrome).
Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

https://reviewboard.mozilla.org/r/77068/#review75774

At this specific commit it is still running in the Chrome process.
The comment is updated in the commit called "Use child's Extension instead of the process'".
Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

https://reviewboard.mozilla.org/r/77072/#review75778

The windowId can change when the tab moves. This commit is just transitional, in "IPC" commit I share the tab/window IDs in a different way (hopefully the right way, I'm not completely sure). The xulWindow/xulBrowser here is mainly used for closing the tab.

I locally ran all WebExtension mochitests, and browser_ext_runtime_openOptionsPage_uninstall.js failed at "Add-on manager tab should still be open". So maybe I don't understand the relation between `<browser>`, tabs and windows as well as I thought.
Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

https://reviewboard.mozilla.org/r/77056/#review76124
Attachment #8788664 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

https://reviewboard.mozilla.org/r/77062/#review76126

Is there a reason you need this here, or is it just nice to have?
Attachment #8788667 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

https://reviewboard.mozilla.org/r/77066/#review76128
Attachment #8788669 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

https://reviewboard.mozilla.org/r/77066/#review76130

Actually, now I don't understand why you don't need to waive. How are the Xray wrappers getting bypassed here?
Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

https://reviewboard.mozilla.org/r/77544/#review76132
Attachment #8789268 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

https://reviewboard.mozilla.org/r/77080/#review76134
Attachment #8788676 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

https://reviewboard.mozilla.org/r/77062/#review76126

When I moved all APIs to the ChildAPIManager, the test referenced in commit "Fix timing issue in browser/page action tests" failed (the images were not ready yet). So I modified the test to take the return values as promises. Before this patch, the return value was `undefined`, so the test behavior did not change.

Then I added this patch on top, and the test would pass if I would move all APIs to `ChildAPIManager`.
Due to the many failures, I reverted the patch that moved all APIs and started working on `WannabeChildAPIManager`. The result that you see here is a set of patches that incrementally adds new features, mostly without test failures in every patch.

Now I have `WannabeChildAPIManager` as an escape hatch, I'll retry moving over all APIs.
Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

https://reviewboard.mozilla.org/r/77066/#review76130

Why do you think that Xray wrappers are or should be bypassed?
The guarantees for plain objects seems good enough to implement serialization: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision#Xray_semantics_for_Object_and_Array.

Callable properties still cause an error to be printed in the console and results in the "undefined" value. But since we decided to use undefined to represent non-serializable objects (opposed to empty objects like Chrome), this is not too bad.
(In reply to Rob Wu [:robwu] from comment #68)
> Comment on attachment 8788669 [details]
> Bug 1287007 - Harden browser.storage.local serialization
> 
> https://reviewboard.mozilla.org/r/77066/#review76130
> 
> Why do you think that Xray wrappers are or should be bypassed?
> The guarantees for plain objects seems good enough to implement
> serialization:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/
> Xray_vision#Xray_semantics_for_Object_and_Array.

Oh, right, I forgot about that. Xrays for plain objects are relatively new.
Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

https://reviewboard.mozilla.org/r/77078/#review76406

::: toolkit/components/extensions/ExtensionContent.jsm:15
(Diff revision 2)
>  
>  /*
>   * This file handles the content process side of extensions. It mainly
>   * takes care of content script injection, content script APIs, and
>   * messaging.
> + * This file is also the initial entry point for addon processes.

Please add a blank line in between paragraphs.
Attachment #8788675 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

https://reviewboard.mozilla.org/r/77082/#review76448

I have a bunch of questions here, but it mostly looks good.

::: browser/components/extensions/ext-tabs.js:61
(Diff revision 3)
>  
>  extensions.on("page-shutdown", (type, context) => {
>    if (context.viewType == "tab") {
> -    let {xulWindow, tab} = getDocShellOwner(context.docShell);
> +    let {gBrowser} = context.xulBrowser.ownerGlobal;
> +    if (gBrowser) {
> +      let tab = gBrowser.getTabForBrowser(context.xulBrowser);

This isn't going to work if the tab is moved to a different window. We need to find a fix by either updating xulBrowser when the tab is moved or by finding the tab to close in some other way.

Perhaps we could move the shutdown logic to the add-on process?

::: browser/components/extensions/ext-utils.js:261
(Diff revision 3)
>      // will be if and when we popup debugging.
>  
>      viewNode.appendChild(this.browser);
>  
>      extensions.emit("extension-browser-inserted", this.browser);
> +    let windowId = WindowManager.getId(this.browser.docShell.chromeEventHandler.ownerGlobal);

this.browser.ownerGlobal

::: browser/components/extensions/ext-utils.js:756
(Diff revision 3)
> +    }
> +  },
> +};
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("startup", (type, extension) => {
> +  Services.mm.addMessageListener("Extension:GetTabAndWindowId", onGetTabAndWindowId);

This will get registered every time an extension starts up. Maybe just call addMessageListener at the top level.

::: toolkit/components/extensions/ExtensionChild.jsm:242
(Diff revision 3)
>    }
>  }
>  
> +// All subframes in a tab, background page, popup, etc. have the same view type.
> +// This class keeps track of such global state.
> +class GlobalView {

Let's call this ContentGlobal.

::: toolkit/components/extensions/ExtensionChild.jsm:248
(Diff revision 3)
> +  /**
> +   * @param {nsIContentFrameMessageManager} global The frame script's global.
> +   */
> +  constructor(global) {
> +    this.global = global;
> +    // Unless specified otherwise assume that the extension page is in a tab.

Why? Why not leave it undefined?

Also, one of these objects will be created for each normal web page. We should make that clear in comments and probably set the viewType to something special (like undefined) in that case.

::: toolkit/components/extensions/ExtensionChild.jsm:254
(Diff revision 3)
> +    this.viewType = "tab";
> +    this.tabId = -1;
> +    this.windowId = -1;
> +    this.global.addMessageListener("Extension:InitExtensionView", this);
> +    this.global.addMessageListener("Extension:SetTabAndWindowId", this);
> +    this.global.sendAsyncMessage("Extension:GetTabAndWindowId");

I don't really see what this is for. The result is probably going to come back too late to be useful, and you're already sending down SetTabAndWindowId messages for extension pages.

::: toolkit/components/extensions/ExtensionChild.jsm:373
(Diff revision 3)
> -    let docShell = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> -                                .getInterface(Ci.nsIDocShell);
> -
> -    let parentDocument = docShell.parent.QueryInterface(Ci.nsIDocShell)
> -                                 .contentViewer.DOMDocument;
> -
> +    let mm = contentWindow
> +      .QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDocShell)
> +      .QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIContentFrameMessageManager);
> +    let {viewType, tabId} = this.globalViews.get(mm);

For pop-ups, won't the document-element-inserted observer fire before we get the message that sets the tab ID? In which case the tab/window ID will be wrong here.
Attachment #8788677 - Flags: review?(wmccloskey)
See Also: → 1301862
Depends on: 1301862
See Also: 1301862
Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

https://reviewboard.mozilla.org/r/77082/#review76448

> This isn't going to work if the tab is moved to a different window. We need to find a fix by either updating xulBrowser when the tab is moved or by finding the tab to close in some other way.
> 
> Perhaps we could move the shutdown logic to the add-on process?

I have fixed the xulBrowser state because it is needed for moving all APIs (see new commit + description).

It probably does not hurt to add `context.contentWindow.close()` to the add-on process, but I have not added it so far to make sure that we are not accidentally hiding bugs in the parent process. After all, the child process cannot be trusted as much as the parent process to do The Right Thing.

> Why? Why not leave it undefined?
> 
> Also, one of these objects will be created for each normal web page. We should make that clear in comments and probably set the viewType to something special (like undefined) in that case.

I added a comment to explain why.

> I don't really see what this is for. The result is probably going to come back too late to be useful, and you're already sending down SetTabAndWindowId messages for extension pages.

This is for extension tabs, which have no special initialization logic yet. Added comment.

> For pop-ups, won't the document-element-inserted observer fire before we get the message that sets the tab ID? In which case the tab/window ID will be wrong here.

Not in the cases that we care about.
If the popup has no URL, then I don't care about the correctness of the window/tabIds.
If the popup does have a URL, then it currently waits until the initial about:blank page is loaded before committing the new URL. That is more than enough time to process the viewType message.

See http://searchfox.org/mozilla-central/rev/124c120ce9d7e8407c9ad330a9d6b1c4ad432637/browser/components/extensions/ext-utils.js#270
Duplicate of this bug: 1299411
Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

https://reviewboard.mozilla.org/r/77082/#review77474

::: toolkit/components/extensions/ExtensionChild.jsm:381
(Diff revision 4)
> -    let docShell = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> -                                .getInterface(Ci.nsIDocShell);
> -
> -    let parentDocument = docShell.parent.QueryInterface(Ci.nsIDocShell)
> -                                 .contentViewer.DOMDocument;
> -
> +    let mm = contentWindow
> +      .QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDocShell)
> +      .QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIContentFrameMessageManager);
> +    let {viewType, tabId} = this.contentGlobals.get(mm);

Let's send a sync message here to get the tab ID when it's missing.
Attachment #8788677 - Flags: review?(wmccloskey) → review+
Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

https://reviewboard.mozilla.org/r/77546/#review77504
Attachment #8789269 - Flags: review?(wmccloskey) → review+
Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

https://reviewboard.mozilla.org/r/77084/#review77506

::: toolkit/components/extensions/ExtensionChild.jsm:132
(Diff revision 4)
> +  /**
> +   * This ExtensionContext represents a privileged addon execution environment
> +   * that has full access to the WebExtensions APIs (provided that the correct
> +   * permissions have been requested).
> +   *
> +   * @param {BrowserExtensionContent|Extension} extension This context's owner.

Isn't the point of this patch that it's always a BrowserExtensionContent now? When is it ever an Extension?
Attachment #8788678 - Flags: review?(wmccloskey) → review+
Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

https://reviewboard.mozilla.org/r/77550/#review77510
Attachment #8789271 - Flags: review?(wmccloskey) → review+
Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

https://reviewboard.mozilla.org/r/77810/#review77684
Attachment #8789674 - Flags: review?(wmccloskey) → review+
Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

https://reviewboard.mozilla.org/r/77812/#review77686
Attachment #8789675 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

https://reviewboard.mozilla.org/r/78674/#review78142
Attachment #8791158 - Flags: review?(bob.silverberg) → review+
Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

https://reviewboard.mozilla.org/r/78662/#review78906

::: toolkit/components/extensions/Extension.jsm:261
(Diff revision 2)
> +    if (this._isSwapping) {
> +      return;
> +    }
> +    this._isSwapping = true;
> +    Promise.resolve().then(() => { this._isSwapping = false; });

I replied in bug 1301837 about this. I don't think we need _isSwapping or the microtask. However, it seems like you need to add a SwapDocShells listener to otherBrowser. Otherwise I think you'll miss future swaps.
Attachment #8791152 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

https://reviewboard.mozilla.org/r/78664/#review78908
Attachment #8791153 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

https://reviewboard.mozilla.org/r/78666/#review78910

::: toolkit/components/extensions/ExtensionUtils.jsm:540
(Diff revision 2)
>            // The Chrome documentation specifies these parameters as
>            // relative paths. We currently accept absolute URLs as well,
>            // which means we need to check that the extension is allowed
>            // to load them. This will throw an error if it's not allowed.
>            Services.scriptSecurityManager.checkLoadURIStrWithPrincipal(
> -            extension.principal, url,
> +            principal, url,

Does this change matter? I would assume we just use the origin of the principal here, and I'd expect that context.principal has the same origin as extension.principal. Is that not true?
Attachment #8791154 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

https://reviewboard.mozilla.org/r/78670/#review78912
Attachment #8791156 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

https://reviewboard.mozilla.org/r/78672/#review78914
Attachment #8791157 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791161 [details]
Bug 1287007 - Move native messaging to child process

https://reviewboard.mozilla.org/r/78680/#review79100

Sorry this has taken so long, but I'm still having a hard time reviewing this patch, there's just way too much going on in it.  The main thing is related to OOP of course but you're also doing other refactoring along the way and also changing the semantics of some of the api methods (ie disconnect) along the way.  Can you split this up so that you have a few smaller patches, doing one of those things at a time?  (it doesn't look like there's any reason they can't be done independently)
Also the parent bug already has a ton of patches, I think that's a sign that the parent bug should be broken into a few smaller bugs so the individual pieces can be reviewed and landed independently.

::: toolkit/components/extensions/ext-c-runtime.js:60
(Diff revision 2)
>          let recipient = {extensionId};
>  
>          return context.messenger.sendMessage(context.messageManager, message, recipient, responseCallback);
>        },
>  
> +      connectNative(application) {

I'm confused by this, I thought ext-c-runtime.js was for apis exposed to content scripts?

::: toolkit/components/extensions/ext-runtime.js
(Diff revision 2)
> -        let app = new NativeApp(extension, context, application);
> -        return app.sendMessage(message);
> -      },
> -
>        get lastError() {
> -        // TODO(robwu): Figure out how to make sure that errors in the parent

why are you removing this comment?
if this is addressed by a different patch, this change should be part of that patch...
Attachment #8791161 - Flags: review?(aswan) → review-
Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

https://reviewboard.mozilla.org/r/78668/#review79282

::: browser/components/extensions/ext-c-contextMenus.js:110
(Diff revision 2)
> +  let onClickedProp = new ContextMenusClickPropHandler(context);
> +
> +  return {
> +    contextMenus: {
> +      create(createProperties, callback) {
> +        if (typeof createProperties.id != "string") {

Can't we say |if (createProperties.id !== null)|? The schema says it must be a string.
Attachment #8791155 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

https://reviewboard.mozilla.org/r/78676/#review79328

::: browser/components/extensions/ext-utils.js:98
(Diff revision 2)
>  
>  /* eslint-disable mozilla/balanced-listeners */
>  extensions.on("page-shutdown", (type, context) => {
>    if (context.viewType == "popup" && context.active) {
> -    context.contentWindow.close();
> +    // TODO(robwu): This is not webext-oop compatible.
> +    context.xulBrowser.contentWindow.close();

As we discussed, I think you should be able to do something like:
  context.xulBrowser.parentNode.hidePopup();
The parentNode should be the XUL panel for the popup.

::: toolkit/components/extensions/ExtensionUtils.jsm:1056
(Diff revision 2)
>        let msg = `In add-on ${id}, attempting to use listener "${name}", which is unimplemented.`;
> -      let winID = context.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> -        .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;
>        let scriptError = Cc["@mozilla.org/scripterror;1"]
>          .createInstance(Ci.nsIScriptError);
>        scriptError.initWithWindowID(msg, frame.filename, null,

If the window is in a different process, there's no point in calling initWithWindowID. Better just to call init and forget about passing around the window ID. It doesn't really matter much as far as I can tell.
Attachment #8791159 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

https://reviewboard.mozilla.org/r/78678/#review79330

::: browser/components/extensions/ext-tabs.js:211
(Diff revision 2)
> +    // When addons run in the main process, `window.close()` is synchronous. So
> +    // wait until all messages to the message manager have flushed, in case the
> +    // extension tried to register a new listener to tabs.onRemoved right before
> +    // calling `window.close()`.

This is not a great way of explaining. How about this:
"When addons run in-process, `window.close()` is synchronous. Most other addon-invoked calls are asynchronous since they go through a proxy context via the message manager. To avoid `window.close()` executing before something that was called before it, it must also run asynchronously."
Attachment #8791160 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

https://reviewboard.mozilla.org/r/79200/#review79332
Attachment #8791930 - Flags: review?(wmccloskey) → review+
Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

https://reviewboard.mozilla.org/r/78666/#review78910

> Does this change matter? I would assume we just use the origin of the principal here, and I'd expect that context.principal has the same origin as extension.principal. Is that not true?

In child processes, the `extension` here is a `BrowserExtensionContent` from ExtensionContent.jsm. That does not have a `principal` property.
Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

https://reviewboard.mozilla.org/r/78676/#review79328

> As we discussed, I think you should be able to do something like:
>   context.xulBrowser.parentNode.hidePopup();
> The parentNode should be the XUL panel for the popup.

The behavior of directly calling `hidePopup()` may differ from a call to `window.close()` (`window.close()` is combined with waiting until the popup becomes visible), so I want to save this change for when the popup (init/resize) logic is changed and becomes fully compatible with webext-oop.
Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

https://reviewboard.mozilla.org/r/78678/#review79330

> This is not a great way of explaining. How about this:
> "When addons run in-process, `window.close()` is synchronous. Most other addon-invoked calls are asynchronous since they go through a proxy context via the message manager. To avoid `window.close()` executing before something that was called before it, it must also run asynchronously."

`window.close` is not run asynchronously. What's being deferred here is the `tabs.onRemoved` event notification, to give the `tabs.onRemoved.addListener` call a chance to be processed before the event is dispatched. I rephrased the comment in an attempt to to communicate this more clearly.
Comment on attachment 8791161 [details]
Bug 1287007 - Move native messaging to child process

https://reviewboard.mozilla.org/r/78680/#review79100

I'll split this up and move it to a separate bug.

> I'm confused by this, I thought ext-c-runtime.js was for apis exposed to content scripts?

ext-c is for anything that runs in a child process.

> why are you removing this comment?
> if this is addressed by a different patch, this change should be part of that patch...

I have since learned that this is correctly handled by ParentAPIManager (which already existed when I put in the TODO). Since I'm touching this file anyway, I decided to remove the TODO since no further action is needed.
(In reply to Rob Wu [:robwu] from comment #151)
> Comment on attachment 8791154 [details]
> Bug 1287007 - Move part of browserAction and pageAction to child
> 
> https://reviewboard.mozilla.org/r/78666/#review78910
> 
> > Does this change matter? I would assume we just use the origin of the principal here, and I'd expect that context.principal has the same origin as extension.principal. Is that not true?
> 
> In child processes, the `extension` here is a `BrowserExtensionContent` from
> ExtensionContent.jsm. That does not have a `principal` property.

Perhaps we could add such a property?
Attachment #8791161 - Attachment is obsolete: true
Blocks: 1305216
Blocks: 1305217
Blocks: 1299411
Comment on attachment 8794528 [details]
Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

https://reviewboard.mozilla.org/r/80956/#review79716
Attachment #8794528 - Flags: review?(rhelmer) → review+
All patches have r+. Now I'm just waiting until the patch for bug 1301862 is approved, then these patches can be landed.

After landing the patches, the native messaging API needs to be migrated (bug 1299411 - under review), webNavigation needs to be made compatible (bug 1305216 - not started yet) and webRequest as well (bug 1305217 - not started yet). Also the popup logic (resizing / closing) should be changed to not directly interact with the content (no bug yet).
Hi Rob,
Have you gotten a green try run yet? We've talked over the best way to land this. If we set shouldSynchronouslyUseParentAPI to true for all APIs, I suppose we could land it very soon? I filed bug 1302629 to make is easier to test with shouldSynchronouslyUseParentAPI set to both true and false. Kris has agreed to handle getting the tests working somehow (even if we have to hack something together).

Do you think you'll have time to rebase this and get it ready for landing?
Flags: needinfo?(rob)
In reply to Bill McCloskey (:billm) from comment #200)
> Hi Rob,
> Have you gotten a green try run yet?

The most recent try job (=reviewboard listing minus last patch, plus native messaging patch that was moved to bug 1299411) is https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5df36be033a.
It includes the patch from bug 1301862, as I mentioned in comment 199 (awaiting review from Kris).
The try has two WebExtensions failures:
- browser/components/extensions/test/browser/browser_ext_windows_update.js - Cannot reproduce locally on a Mac (with and without --disable-e10s, and the there are already existing reports about intermittents, e.g. bug 1255983, so I did not investigate this further).
- toolkit/mozapps/extensions/test/xpcshell/test_delay_update_webextension.js - added patch to fix test failure: https://reviewboard.mozilla.org/r/80956/diff/1#index_header

Once Kris has reviewed my patch for bug 1301862, I'll rebase and trigger a try from reviewboard. I expect everything to be green without any other changes.


> We've talked over the best way to land this. If we set shouldSynchronouslyUseParentAPI
> to true for all APIs, I suppose we could land it very soon?
> I filed bug 1302629 to make is easier to
> test with shouldSynchronouslyUseParentAPI set to both true and false.

When shouldSynchronouslyUseParentAPI is true, the implementation ought to behave as if all these patches were not applied (test and storage already go through the ChildAPIManager at the moment, so maybe we have to only set the flag to true when these APIs are not used if we really want to preserve the current behavior as much as possible).

The only reason that I can imagine for setting this to true is that routing every call through the message managers (via ChildAPIManager/ParentAPIManager) incurs overhead, and until add-ons really run out-of-process, the overhead has no use.

A benefit of having a test where this flag is forced to true is that we get additional test coverage to expose timing issues.
For users (addon devs) toggling this flag should not have any perceivable difference in behavior.

If we add a test for the boolean then we will have two code paths, one where everything is proxied and one where it is not. If the cost of maintenance becomes unbearable there is no objection to dropping the test that sets the flag to true.

I wanted to start with flipping the flag anyway so that we have the test infrastructure in place when we want to try and run add-ons in a separate process.


> Kris has agreed to handle getting the tests working somehow (even if we have to
> hack something together).

If I understood you correctly, "getting the tests working" means to have a setup where we can run all tests twice (with different values for some boolean pref), right?

> Do you think you'll have time to rebase this and get it ready for landing?

Yes, as soon as the patch for bug 1301862 lands. I will wait a few days to make sure that the test is not backed out.

Once all APIs are certainly oop-compatible, we can get rid of the WannabeChildAPIManager and use the simplified ChildAPIManager instead. As point of interest as a part of this clean-up is that the WannabeChildAPIManager constructs/destructs the ProxyContext synchronously, whereas with ChildAPIManager it is asynchronously. I haven't checked yet, but some tests that rely on tight timing might fail as a result. This is not difficult to solve if it is an issue.
Flags: needinfo?(rob)
Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

https://reviewboard.mozilla.org/r/77066/#review80802

::: toolkit/components/extensions/ExtensionStorage.jsm:45
(Diff revision 4)
> + *
> + * @param {*} value An untrusted object supplied by an addon.
> + * @param {AntiCycleStack} stack An auxilary stack to detect cyclic references.
> + * @returns {*} A safe JSON-compatible representation of this object.
> + */
> +function sanitize(value, stack) {

I'd rather we keep the `JSON.stringify` implementation of this. Respecting methods like `toJSON` defined in the target slope was intentional, and the X-ray warnings for functions were expected.

If you still want to make these serialization changes, please file a separate bug, since they're entirely unrelated to what you need to do here.
Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

https://reviewboard.mozilla.org/r/77066/#review80802

> I'd rather we keep the `JSON.stringify` implementation of this. Respecting methods like `toJSON` defined in the target slope was intentional, and the X-ray warnings for functions were expected.
> 
> If you still want to make these serialization changes, please file a separate bug, since they're entirely unrelated to what you need to do here.

I reverted the change, as requested.

Note that `toJSON` was currently only respected for code running in the main process, *not* for content scripts (this is a result of IPC).
Before my changes:

```
browser.storage.local.set({toJSON(){return 1}})
  // Content: Error in global console: Sending message that cannot be cloned. Are you trying to send an XPCOM object?
  //  (because functions cannot be serialized over IPC, i.e. sendAsyncMeessage of the message manager)
  // Background: Creates {toJSON: {}} (i.e. "toJSON" key with value {}) // ...?

browser.storage.local.set({ a:{m:3,toJSON(){return 4}} })
  // Content: creates {a:{m:3}}
  // + error in global console: XrayWrapper denied access to property "toJSON" (reason: value is callable). See https://developer.mozilla.org/en-US/docs/Xray_vision for more information. Note that only the first denied property access from a given global object will be reported.
  //  (again, functions cannot be sent over IPC).
  // Background: creates {a:4}
```

After the latest changes, the storage API in the content script behaves like the background page.
(my original proposal unified the behavior in the other way around, i.e. in both cases toJSON was ignored).
Attachment #8796629 - Flags: review?(wmccloskey) → review?(kmaglione+bmo)
Comment on attachment 8796950 [details]
Bug 1287007 - Remove contextMenusInternal.onClicked

https://reviewboard.mozilla.org/r/82600/#review81482
Attachment #8796950 - Flags: review?(wmccloskey) → review+
Comment on attachment 8796951 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_onUpdated.js

https://reviewboard.mozilla.org/r/82602/#review81484
Attachment #8796951 - Flags: review?(wmccloskey) → review+
Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

https://reviewboard.mozilla.org/r/82418/#review81530
Attachment #8796629 - Flags: review?(kmaglione+bmo) → review+
Depends on: 1307759