1287007 - Start using ChildAPIManager for some APIs

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

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

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

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Use minimal global scope for ext- scripts

This is to prepare moving the API generation logic from Extension.jsm to
ExtensionUtils.jsm, so it can be used by content and addon processes.

ExtensionUtils.jsm declares EventEmitter in its local scope, while
scripts such as ext-tabs.js rely on the API of another EventEmitter
(from devtools, which has EventEmitter.decorate). To fix the problem,
I introduce a new JSM that has an own scope and use that scope as the
global for ext-*.js modules.

For now all tests pass, but ideally all Cu.import without second
argument should be converted to use the second argument, to prevent
undesirable non-explicit dependencies.

Review commit: https://reviewboard.mozilla.org/r/72736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72736/

Comment 2

[Rob Wu [:robwu]]

Comment on attachment 8782677 [details]
Bug 1287007 - Use minimal global scope for ext- scripts

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72736/diff/1-2/

Comment 3

[Rob Wu [:robwu]]

Comment on attachment 8782677 [details]
Bug 1287007 - Use minimal global scope for ext- scripts

Patch was associated with the wrong bug.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix some flaws in ProxyAPIImplementation

- removeListener: There is no set.remove, use set.delete.
- Async callbacks: Do not unconditionally turn the result in a
  SpreadArgs because it causes the result to unconditionally
  be wrapped in an array.

Review commit: https://reviewboard.mozilla.org/r/77086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77086/

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Improve errors for non-existing remote APIs

Currently, if the remote implementation is missing, the next unhelpful
error message is logged to the console:
"TypeError: findPathInObject(...) is not a function" or
"TypeError: findPathInObject(...) is undefined", etc.

This commit makes the message more useful:
"findPathInObject: No obj found for path: tabs.create"

Review commit: https://reviewboard.mozilla.org/r/77054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77054/

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Require "async" in schemas to match name

=== Before ===
In the pageAction and browserAction schemas, several methods are
declared with `"async": true` but without a specified callback in the
`"parameters"` object, so callbacks are not allowed. However, when a
callback is proxied, the `ParentAPIManager` will mirror the call by
passing in an extra callback to the proxied API - and break.

=== After ===

If `"async": true` is specified in the schema, then there is no named
callback parameter in the "chrome." API. The "browser." API, on the
other hand will either return a promise or accept an extra callback.
Examples of accepted formats:
- chrome.pageAction.show(1)
- browser.pageAction.show(1)
- browser.pageAction.show(1, callback)
- browser.pageAction.show(1).then(callback)
Example of rejected formats:
- chrome.pageAction.show(1, callback)

New tests:
- `testParameterValidation` tests the above scenarios.
- `testAsyncResults` tests whether error propagation works. This did not
  fail before this commit, and still passes with the changes.

Review commit: https://reviewboard.mozilla.org/r/77056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77056/

Comment 7

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Refactor test_ext_schemas_api_injection.js

When the background API move to a separate implementation,
then the schema APIs will be generated (and ChildAPIManager
will just delegate the implementation to the parent process).

The purpose of the test is to verify that nested namespaces and
null-feturn values work, so just use the base classes instead of a
concrete implementation for BaseContext / SchemaAPIManager instead of
setting up a full extension.

Review commit: https://reviewboard.mozilla.org/r/77058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77058/

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix timing issue in browser/page action tests

Review commit: https://reviewboard.mozilla.org/r/77060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77060/

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Mark all browserAction methods as async

E.g. browser.browserAction.enable(...).then(...) now works as expected.

Review commit: https://reviewboard.mozilla.org/r/77062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77062/

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Allow local implementations to call remote implementations

- Add callParentFunctionNoReturn / callParentAsyncFunction to
  ChildAPIManager to implement remote calls.

- Add in-process browser.test implementation that uses this.

- Add tests to verify that the browser.test.assert* methods with
  the `allowAmbiguousOptionalArguments` schema attribute are working
  with objects that cannot be passed as-is over IPC.
  (except test.sendMessage, because stringifying the arguments has an
   observable impact on test behavior)

Review commit: https://reviewboard.mozilla.org/r/77064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77064/

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix cross-process browser.storage.local serialization

- Lazily initialize file IO-specific stuff in ExtensionStorage.jsm,
  and limit this work to the main process.

- Add local versions of the `storage.local.get` and `storage.local.set`
  implementations that perform sanitization (without the change, values
  are improperly serialized over IPC).

- Switch from JSON.stringify to a custom serializer, because:
  - The serialization of the value is affected by the context's override
    of methods such as `toJSON` and `toString`.
  - Nested objects with function values trigger the following error:

  >  XrayWrapper denied access to property "foo" (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.

- Copied the `backgroundScript` test from xpcshell/test_ext_storage.js
  to mochitest/test_ext_storage_content.html because they should behave
  identical. Before this patch the test failed due to IPC serialization
  issues, now the test passes.
  Note that the old test also passes with the changes.

Review commit: https://reviewboard.mozilla.org/r/77066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77066/

Comment 12

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Move ExtensionContext to separate file

This is just a mechanical change, literally nothing more than cutting
ExtensionContext from Extension.jsm, pasting it in ExtensionChild.jsm
and adding the minimal imoort boilerplate.

Review commit: https://reviewboard.mozilla.org/r/77068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77068/

Comment 13

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Do not neuter error messages from the same principal

With an upcoming change that introduces another ProxyContext,
errors from a different scope may bubble up. These messages should
be reported, not replaced with "An unexpected error occurred".

Review commit: https://reviewboard.mozilla.org/r/77070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77070/

Comment 14

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Introduce ChildAPIManager to addon code

This is the bare minimum to separate the generation of addon_parent and
addon_child APIs. Now it is possible to have methods with the same name
but different implementations in the parent and child.

Many APIs are not compatible with the proxied API implementation, so
they temporarily fall back to directly invoking the parent API, just as
before this commit.

Review commit: https://reviewboard.mozilla.org/r/77072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77072/

Comment 15

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Rename "context.type" to "context.viewType"

"viewType" is more easily searchable and not as ambiguous as "type".

Review commit: https://reviewboard.mozilla.org/r/77074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77074/

Comment 16

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Move background page API logic to child

Review commit: https://reviewboard.mozilla.org/r/77076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77076/

Comment 17

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Move extension context initialization to ExtensionContent

This is a simple move of ExtensionContext creation logic to
ExtensionChild.

Before the change, ExtensionContext was initialized as follows:

1. (ext-backgroundPage.js) Create background page
2. (Extension.jsm) document-element-inserted observed.
3. (Extension.jsm) new ExtensionContext + unload observer.

After this commit:

1. (ext-backgroundPage.js) Create background page
2. (ext-backgroundPage.js) emit extension-browser-inserted event
3. (Extension.jsm) Pass global to ExtensionContent + unload listener.
4. (ExtensionContent.jsm) document-element-inserted observed.
5. (ExtensionChild.jsm) new ExtensionContext

The next step is to use frame scripts and synchronize state.

Review commit: https://reviewboard.mozilla.org/r/77078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77078/

Comment 18

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

browser.test.sendMessage goes through the ChildAPIManager,
whereas tabs.remove is currently a direct call

Review commit: https://reviewboard.mozilla.org/r/77080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77080/

Comment 19

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Use IPC to share viewType, tabId and windowId

Accessing <browser> in ContentChild does not work when extensions run in
a separate process.

Review commit: https://reviewboard.mozilla.org/r/77082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77082/

Comment 20

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Use child's Extension instead of the process'

Review commit: https://reviewboard.mozilla.org/r/77084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77084/

Comment 21

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

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.

Comment 22

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

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).`?

Comment 23

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

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.

Comment 24

[Rob Wu [:robwu]]

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 25

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

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.

Comment 26

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

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"

Comment 27

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

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.

Comment 28

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

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

https://reviewboard.mozilla.org/r/77064/#review75770

Comment 29

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

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.

Comment 30

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

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.

Comment 31

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

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

https://reviewboard.mozilla.org/r/77070/#review75776

Comment 32

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

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.

Comment 33

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

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

https://reviewboard.mozilla.org/r/77074/#review75780

Comment 34

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

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.

Comment 35

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

This is as far as I got on the plane. I'll get to the rest as soon as I can.

Comment 36

[Rob Wu [:robwu]]

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 37

[Rob Wu [:robwu]]

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 38

[Rob Wu [:robwu]]

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 39

[Rob Wu [:robwu]]

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 40

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Move extension.getViews to child

Review commit: https://reviewboard.mozilla.org/r/77544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77544/

Comment 41

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix test_ext_api_permissions.js

"extension.views" will only be available in content, so
use the "proxy-context-load" event to detect new views.

Review commit: https://reviewboard.mozilla.org/r/77546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77546/

Comment 42

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Remove dead code

Review commit: https://reviewboard.mozilla.org/r/77548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77548/

Comment 43

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Set the principal in ChildAPIManager

The `browser.downloads.download` API takes a "url" parameter. The schema
file at downloads.json defines the type to be "url". This means that the
parameter is checked with `context.checkLoadURL(url)` in Schemas.jsm.
This method uses the principal of the object that was passed to
`Schemas.inject`.

Currently, this works just fine because the `schemaWrapper` in
Extension.jsm returns the context's principal.
But when we move to using the ChildAPIManager, the principal is not
defined and Schemas.jsm will fall back to a Null principal.

As a result, the test_ext_downloads_download.js fails because the
blob:-URL with the extension origin cannot be loaded by a null
principal. To fix this, the context's principal must be set.

Review commit: https://reviewboard.mozilla.org/r/77550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77550/

Comment 44

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/1-2/

Comment 45

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/1-2/

Comment 46

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/1-2/

Comment 47

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/1-2/

Comment 48

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/1-2/

Comment 49

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/1-2/

Comment 50

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/1-2/

Comment 51

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/1-2/

Comment 52

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/1-2/

Comment 53

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/1-2/

Comment 54

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/1-2/

Comment 55

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/1-2/

Comment 56

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/1-2/

Comment 57

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/1-2/

Comment 58

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/1-2/

Comment 59

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/1-2/

Comment 60

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/1-2/

Comment 61

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

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

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

Comment 62

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

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?

Comment 63

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

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

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

Comment 64

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

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 65

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

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

https://reviewboard.mozilla.org/r/77544/#review76132

Comment 66

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

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

https://reviewboard.mozilla.org/r/77080/#review76134

Comment 67

[Rob Wu [:robwu]]

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 68

[Rob Wu [:robwu]]

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.

Comment 69

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Only close extension tabs upon shutdown

In one of the previous patches, the viewType of popup changed from
"popup" to "tab". As a result it was closed by the `page-shutdown`
event handler in ext-tabs.js. This prevents that from happening.

Also added a test that checks whether the options page type is a tab, to
prevent future regressions.

Review commit: https://reviewboard.mozilla.org/r/77810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77810/

Comment 70

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Use frame script to initialize background/popup page

Review commit: https://reviewboard.mozilla.org/r/77812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77812/

Comment 71

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/2-3/

Comment 72

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/2-3/

Comment 73

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/1-2/

Comment 74

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/2-3/

Comment 75

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/1-2/

Comment 76

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/1-2/

Comment 77

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

(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 78

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

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.

Comment 79

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

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.

Comment 80

[Rob Wu [:robwu]]

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

Comment 81

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Track message manager / browser swaps

Neither the message manager nor the XUL browser is guaranteed to be
constant during a ProxyContext's lifetime.

Add a new class to follow the `<browser>` belonging to the current
docshell and update the ProxyContext properties as needed.

Also, renamed `messageManager` to `currentMessageManager` because the
`messageManager` property is overwritten by the `setContentWindow` hack
in WannabeChildAPIManager in ExtensionChild.jsm.

browser/components/extensions/test/browser/browser_ext_currentWindow.js
provides test coverage for this feature once the `test` API goes through
a ChildAPIManager instead of directly through a WannabeChildAPIManager.
Why? Because that test calls `test.onMessage.addListener` in the script
that is loaded in a popup page. Popups are loaded in two stages: First
the content is preloaded in a `<browser>`, and then when the popup is
shown a new `<browser>` is created and the docshells are swapped.
When the script runs while the popup script is being preloaded, the
`ParentAPIManager` receives the IPC message with the target set to the
`<browser>` used for preloading. When the API response is ready,
`target.messageManager.sendAsyncMessage` is called. Meanwhile the
docshells have been swapped, the message manager is gone and this fails.
With this patch, the message manager is correctly tracked and this test
passes.

Review commit: https://reviewboard.mozilla.org/r/78662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78662/

Comment 82

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Set parent cloneScope to child cloneScope

This is only to help with migration. This change allows all APIs to
behave identical regardless of whether the API is proxied.

Change cloneScope to be a getter because cloneScope is
`this.contentWindow`, which may be nulled when the context navigates away
(but stays in the bfcache).

Any API that is not proxied must have an identical clone scope to make
sure that properties such as toJSON (in the native messaging
stringifier) and ArrayBuffer (in webRequest as requestBody) are visible
to the caller.

Review commit: https://reviewboard.mozilla.org/r/78664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78664/

Comment 83

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Move part of browserAction and pageAction to child

And remove redundant `Promise.resolve()` because it is the default
for async functions.

setIcon is not supported on Android, so there was no need to change
mobile/android/components/extensions/ext-pageAction.js.

Review commit: https://reviewboard.mozilla.org/r/78666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78666/

Comment 84

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix "onclick" in contextMenus, to child.

Main thing: Making contextMenus implementation webext-oop compatible.

Preparation:

- Add getParentEvent to ChildAPIManager to allow use of remote events.
- Introduce `addon_parent_only` "restriction" to only generate a schema
  API in the main process.
- Do not fill in `null` for missing keys if the schema declares a key as
  `"optional": "omit-key-if-missing"`. This is needed for the second
  point in the next list.

Drive-by fixes:

- Ensure that the "onclick" handler is erased when a context closes.
- Do not clear the "onclick" handler in `contextMenus.update` if the
  onclick key has been omitted (parity with Chrome).
- Remove some unnecessary `Promise.resolve()`
- Add extensive set of tests that check the behavior of the contextMenus
  APIs with regards to the onclick attribute in various scenarios.

Review commit: https://reviewboard.mozilla.org/r/78668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78668/

Comment 85

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix timing issue in test_ext_storage.js

Review commit: https://reviewboard.mozilla.org/r/78670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78670/

Comment 86

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Enable proxying of most APIs.

Review commit: https://reviewboard.mozilla.org/r/78672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78672/

Comment 87

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix test_ext_management_uninstall_self.js

The test logic was broken by design: Two tests uninstall the addon,
but only one uninstall observer was used. Consequently, the second
test resumes the test before the addon was actually uninstalled.

It is probably sheer luck that the test worked before.

Review commit: https://reviewboard.mozilla.org/r/78674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78674/

Comment 88

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Remove .contentWindow from ProxyContext

Review commit: https://reviewboard.mozilla.org/r/78676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78676/

Comment 89

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Make window.close in extension pages async

Test coverage by tabs.onRemoved + window.close() in:
toolkit/components/extensions/test/mochitest/test_ext_tab_teardown.html

Review commit: https://reviewboard.mozilla.org/r/78678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78678/

Comment 90

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Move native messaging to child process

Move `runtime.connectNative` and `runtime.sendNativeMessage` to
`addon_child`. Note: This does not change the behavior for launching the
native app, it is still launched from the main process.

Some other significant changes:

ExtensionUtils.Port:
- Refactor Port in ExtensionUtils to allow using Port without wrappers
  via `registerOnMessage` and `registerOnDisconnect`.
- Add "port" parameter to Port's `onMessage` method (Chrome compat).
- Do not clone the message received by Port's `onMessage` (per & Chrome
  compat).

ExtensionUtils.Messenger
- Add connectGetRawPort to allow `postMessage` with already-sanitized
  values without having to use `cloneInto` with `context.cloneScope`.

Native messaging:
- Do not throw an error when `disconnect()` is called on an
  already-disconnected port (Chrome compat).
- Do not dispatch `onDisconnect` when `.disconnect()` was called (Chrome
  compat).
- Use `context.extension` instead of passing the extension separately.
- Serialize the message in the child instead of the main process.

Review commit: https://reviewboard.mozilla.org/r/78680/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78680/

Comment 91

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/2-3/

Comment 92

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/2-3/

Comment 93

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/2-3/

Comment 94

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/2-3/

Comment 95

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/2-3/

Comment 96

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/2-3/

Comment 97

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/2-3/

Comment 98

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/2-3/

Comment 99

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/2-3/

Comment 100

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/2-3/

Comment 101

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/2-3/

Comment 102

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/2-3/

Comment 103

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/2-3/

Comment 104

[Rob Wu [:robwu]]

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77544/diff/1-2/

Comment 105

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/2-3/

Comment 106

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/3-4/

Comment 107

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/3-4/

Comment 108

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/2-3/

Comment 109

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/3-4/

Comment 110

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/2-3/

Comment 111

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/2-3/

Comment 112

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/1-2/

Comment 113

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/1-2/

Comment 115

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

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.

Comment 116

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

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

https://reviewboard.mozilla.org/r/77546/#review77504

Comment 117

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

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?

Comment 118

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

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

https://reviewboard.mozilla.org/r/77548/#review77508

Comment 119

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

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

https://reviewboard.mozilla.org/r/77550/#review77510

Comment 120

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

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

https://reviewboard.mozilla.org/r/77810/#review77684

Comment 121

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

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

https://reviewboard.mozilla.org/r/77812/#review77686

Comment 122

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

The ProxyMessenger registers a listener whenever the first addon
starts. Although the map does not have any listeners any more at
the end of the test, the listener itself is not removed because
the message-manager-close notification is not sent for them.

So do not count these persistent message managers in the test.
The actual message managers of interest are those associated with
the (closed) tab.

Note: When the test is run in isolation, it may still fail due
to bugzil.la/1293583. See bug for work-around if you want to test.

Review commit: https://reviewboard.mozilla.org/r/79200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79200/

Comment 123

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/4-5/

Comment 124

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/3-4/

Comment 125

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/4-5/

Comment 126

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/3-4/

Comment 127

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/3-4/

Comment 128

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/2-3/

Comment 129

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/2-3/

Comment 130

[Rob Wu [:robwu]]

Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78662/diff/1-2/

Comment 131

[Rob Wu [:robwu]]

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78664/diff/1-2/

Comment 132

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/1-2/

Comment 133

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/1-2/

Comment 134

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/1-2/

Comment 135

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/1-2/

Comment 136

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/1-2/

Comment 137

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/1-2/

Comment 138

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/1-2/

Comment 139

[Rob Wu [:robwu]]

Comment on attachment 8791161 [details]
Bug 1287007 - Move native messaging to child process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78680/diff/1-2/

Comment 140

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

https://reviewboard.mozilla.org/r/78674/#review78142

Comment 141

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

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.

Comment 142

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

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

https://reviewboard.mozilla.org/r/78664/#review78908

Comment 143

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

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?

Comment 144

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

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

https://reviewboard.mozilla.org/r/78670/#review78912

Comment 145

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

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

https://reviewboard.mozilla.org/r/78672/#review78914

Comment 146

[Andrew Swan [:aswan]]

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...

Comment 147

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

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.

Comment 148

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

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.

Comment 149

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

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."

Comment 150

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

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

https://reviewboard.mozilla.org/r/79200/#review79332

Comment 151

[Rob Wu [:robwu]]

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 152

[Rob Wu [:robwu]]

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 153

[Rob Wu [:robwu]]

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 154

[Rob Wu [:robwu]]

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.

Comment 155

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

(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?

Comment 156

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/3-4/

Comment 157

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/3-4/

Comment 158

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/3-4/

Comment 159

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/3-4/

Comment 160

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/3-4/

Comment 161

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/3-4/

Comment 162

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/3-4/

Comment 163

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/3-4/

Comment 164

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/3-4/

Comment 165

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/3-4/

Comment 166

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/3-4/

Comment 167

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/3-4/

Comment 168

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/3-4/

Comment 169

[Rob Wu [:robwu]]

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77544/diff/2-3/

Comment 170

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/3-4/

Comment 171

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/4-5/

Comment 172

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/5-6/

Comment 173

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/4-5/

Comment 174

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/5-6/

Comment 175

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/4-5/

Comment 176

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/4-5/

Comment 177

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/3-4/

Comment 178

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/3-4/

Comment 179

[Rob Wu [:robwu]]

Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78662/diff/2-3/

Comment 180

[Rob Wu [:robwu]]

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78664/diff/2-3/

Comment 181

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/2-3/

Comment 182

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/2-3/

Comment 183

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/2-3/

Comment 184

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/2-3/

Comment 185

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/2-3/

Comment 186

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/2-3/

Comment 187

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/2-3/

Comment 188

[Rob Wu [:robwu]]

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79200/diff/1-2/

Comment 189

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

The onUpdateAvailable listener is asynchronously notified, so the caller
(i.e. the test) cannot assume that a message sent immediately after
triggering an update would trigger the `browser.test.onMessage` listener
that was added in the onUpdateAvailable event handler.

Review commit: https://reviewboard.mozilla.org/r/80956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80956/

Comment 190

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/3-4/

Comment 191

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/3-4/

Comment 192

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/3-4/

Comment 193

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/3-4/

Comment 194

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/3-4/

Comment 195

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/3-4/

Comment 196

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/3-4/

Comment 197

[Rob Wu [:robwu]]

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79200/diff/2-3/

Comment 198

[Robert Helmer [:rhelmer]]

Comment on attachment 8794528 [details]
Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

https://reviewboard.mozilla.org/r/80956/#review79716

Comment 199

[Rob Wu [:robwu]]

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).

Comment 200

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

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?

Comment 201

[Rob Wu [:robwu]]

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.

Comment 202

[Kris Maglione [:kmag]]

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 203

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Derive context.principal from sandbox

`context.principal` should be equal to the principal of the sandbox, so
that if a new sandbox is created using `Cu.Sandbox(principal)`, that
objects can be shared between the new sandbox and `context.cloneScope`
(= `context.sandbox`) without issues.

Without this change, using `context.jsonStringify` on an object from a
content script would trigger the following error:

> Error: Permission denied to access property "toJSON"

This scenario is covered by the test
toolkit/components/extensions/test/mochitest/test_ext_storage_content.html
in the next commit.

Review commit: https://reviewboard.mozilla.org/r/82418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82418/

Comment 204

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/4-5/

Comment 205

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/4-5/

Comment 206

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/4-5/

Comment 207

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/4-5/

Comment 208

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/4-5/

Comment 209

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/4-5/

Comment 210

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/4-5/

Comment 211

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/4-5/

Comment 212

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/4-5/

Comment 213

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/4-5/

Comment 214

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/4-5/

Comment 215

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/4-5/

Comment 216

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/4-5/

Comment 217

[Rob Wu [:robwu]]

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77544/diff/3-4/

Comment 218

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/4-5/

Comment 219

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/5-6/

Comment 220

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/6-7/

Comment 221

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/5-6/

Comment 222

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/6-7/

Comment 223

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/5-6/

Comment 224

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/5-6/

Comment 225

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/4-5/

Comment 226

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/4-5/

Comment 227

[Rob Wu [:robwu]]

Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78662/diff/3-4/

Comment 228

[Rob Wu [:robwu]]

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78664/diff/3-4/

Comment 229

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/4-5/

Comment 230

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/4-5/

Comment 231

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/4-5/

Comment 232

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/4-5/

Comment 233

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/4-5/

Comment 234

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/4-5/

Comment 235

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/4-5/

Comment 236

[Rob Wu [:robwu]]

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79200/diff/3-4/

Comment 237

[Rob Wu [:robwu]]

Comment on attachment 8794528 [details]
Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80956/diff/1-2/

Comment 238

[Rob Wu [:robwu]]

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).

Comment 239

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Remove contextMenusInternal.onClicked

contextMenusInternal.onClicked is already inlined in context_menus.json.
It should be removed because otherwise the
`browser.contextMenusInternal.onClicked` API would be generated.

Review commit: https://reviewboard.mozilla.org/r/82600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82600/

Comment 240

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix timing issue in browser_ext_tabs_onUpdated.js

Reload about:blank tab after opening it and registering the
`tabs.onUpdated` event. This is needed because about:blank loads very
fast, before the callback of `tabs.create` is invoked. This effect is
amplified by the fact that the APIs are now proxied.

Review commit: https://reviewboard.mozilla.org/r/82602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82602/

Comment 241

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

Comment on attachment 8796950 [details]
Bug 1287007 - Remove contextMenusInternal.onClicked

https://reviewboard.mozilla.org/r/82600/#review81482

Comment 242

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

Comment on attachment 8796951 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_onUpdated.js

https://reviewboard.mozilla.org/r/82602/#review81484

Comment 243

[Kris Maglione [:kmag]]

Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

https://reviewboard.mozilla.org/r/82418/#review81530

Comment 244

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Adjust state test in browser_ext_windows_update.js on Linux

The test/browser/browser_ext_windows_update.js test fails on Linux.

The window ID is not expected to change, add assertion for it to
eliminate causes for test failures.

Ultimately, it turns out that the test failure is caused by a defect
in the implementation, not the test itself, see bugzil.la/1307759

Review commit: https://reviewboard.mozilla.org/r/85018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85018/

Comment 245

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/5-6/

Comment 246

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/5-6/

Comment 247

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/5-6/

Comment 248

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/5-6/

Comment 249

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/5-6/

Comment 250

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/5-6/

Comment 251

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/5-6/

Comment 252

[Rob Wu [:robwu]]

Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82418/diff/1-2/

Comment 253

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/5-6/

Comment 254

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/5-6/

Comment 255

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/5-6/

Comment 256

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/5-6/

Comment 257

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/5-6/

Comment 258

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/5-6/

Comment 259

[Rob Wu [:robwu]]

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77544/diff/4-5/

Comment 260

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/5-6/

Comment 261

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/6-7/

Comment 262

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/7-8/

Comment 263

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/6-7/

Comment 264

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/7-8/

Comment 265

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/6-7/

Comment 266

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/6-7/

Comment 267

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/5-6/

Comment 268

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/5-6/

Comment 269

[Rob Wu [:robwu]]

Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78662/diff/4-5/

Comment 270

[Rob Wu [:robwu]]

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78664/diff/4-5/

Comment 271

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/5-6/

Comment 272

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/5-6/

Comment 273

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/5-6/

Comment 274

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/5-6/

Comment 275

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/5-6/

Comment 276

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/5-6/

Comment 277

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/5-6/

Comment 278

[Rob Wu [:robwu]]

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79200/diff/4-5/

Comment 279

[Rob Wu [:robwu]]

Comment on attachment 8794528 [details]
Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80956/diff/2-3/

Comment 280

[Rob Wu [:robwu]]

Comment on attachment 8796950 [details]
Bug 1287007 - Remove contextMenusInternal.onClicked

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82600/diff/1-2/

Comment 281

[Rob Wu [:robwu]]

Comment on attachment 8796951 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_onUpdated.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82602/diff/1-2/

Comment 282

[Kris Maglione [:kmag]]

Comment on attachment 8799980 [details]
Bug 1287007 - Adjust state test in browser_ext_windows_update.js on Linux

https://reviewboard.mozilla.org/r/85018/#review83582

::: browser/components/extensions/test/browser/browser.ini:102
(Diff revision 1)
>  [browser_ext_windows_create_url.js]
>  [browser_ext_windows_events.js]
>  [browser_ext_windows_size.js]
>  skip-if = os == 'mac' # Fails when windows are randomly opened in fullscreen mode
>  [browser_ext_windows_update.js]
> +skip-if = os == 'linux' # windows.update({state}) is broken (bug 1307759).

Please only disable the failing task, not the entire test file.

Comment 283

[Rob Wu [:robwu]]

Comment on attachment 8799980 [details]
Bug 1287007 - Adjust state test in browser_ext_windows_update.js on Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85018/diff/1-2/

Comment 284

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

Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

https://reviewboard.mozilla.org/r/82418/#review83674

It looks like Kris already reviewed this and it hasn't changed since then. Am I missing something?

Comment 285

[Kris Maglione [:kmag]]

Comment on attachment 8799980 [details]
Bug 1287007 - Adjust state test in browser_ext_windows_update.js on Linux

https://reviewboard.mozilla.org/r/85018/#review83864

Comment 286

[Rob Wu [:robwu]]

Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

https://reviewboard.mozilla.org/r/82418/#review83674

I guess that the `r?billm` in the commit title triggered review again. I've amended the commits to remove it.

Comment 287

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix timing issue in browser_ext_contextMenus.js

Review commit: https://reviewboard.mozilla.org/r/85304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85304/

Comment 288

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Work-around for test failure in test_ext_cookies.html

There is a timing-sensitive bug in window.create and/or cookies API.
I haven't figured out the exact details, but created a minimal
test case and reported it at bugzil.la/1309637

Without this patch, the test will fail as described in that bug report.

Review commit: https://reviewboard.mozilla.org/r/85306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85306/

Comment 289

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/6-7/

Comment 290

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/6-7/

Comment 291

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/6-7/

Comment 292

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/6-7/

Comment 293

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/6-7/

Comment 294

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/6-7/

Comment 295

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/6-7/

Comment 296

[Rob Wu [:robwu]]

Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82418/diff/2-3/

Comment 297

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/6-7/

Comment 298

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/6-7/

Comment 299

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/6-7/

Comment 300

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/6-7/

Comment 301

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/6-7/

Comment 302

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/6-7/

Comment 303

[Rob Wu [:robwu]]

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77544/diff/5-6/

Comment 304

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/6-7/

Comment 305

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/7-8/

Comment 306

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/8-9/

Comment 307

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/7-8/

Comment 308

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/8-9/

Comment 309

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/7-8/

Comment 310

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/7-8/

Comment 311

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/6-7/

Comment 312

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/6-7/

Comment 313

[Rob Wu [:robwu]]

Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78662/diff/5-6/

Comment 314

[Rob Wu [:robwu]]

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78664/diff/5-6/

Comment 315

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/6-7/

Comment 316

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/6-7/

Comment 317

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/6-7/

Comment 318

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/6-7/

Comment 319

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/6-7/

Comment 320

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/6-7/

Comment 321

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/6-7/

Comment 322

[Rob Wu [:robwu]]

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79200/diff/5-6/

Comment 323

[Rob Wu [:robwu]]

Comment on attachment 8794528 [details]
Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80956/diff/3-4/

Comment 324

[Rob Wu [:robwu]]

Comment on attachment 8796950 [details]
Bug 1287007 - Remove contextMenusInternal.onClicked

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82600/diff/2-3/

Comment 325

[Rob Wu [:robwu]]

Comment on attachment 8796951 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_onUpdated.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82602/diff/2-3/

Comment 326

[Rob Wu [:robwu]]

Comment on attachment 8799980 [details]
Bug 1287007 - Adjust state test in browser_ext_windows_update.js on Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85018/diff/2-3/

Comment 327

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

Comment on attachment 8800398 [details]
Bug 1287007 - Fix timing issue in browser_ext_contextMenus.js

https://reviewboard.mozilla.org/r/85304/#review83904

Comment 328

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

Comment on attachment 8800399 [details]
Bug 1287007 - Work-around for test failure in test_ext_cookies.html

https://reviewboard.mozilla.org/r/85306/#review83906

Comment 329

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

Hi Rob,
How is this going? Do you need any help? The longer we wait, the harder it will be to rebase these patches.

Comment 330

[Rob Wu [:robwu]]

Hi Bill, in terms of functionality there are no changes. I'm just adjusting tests based on observed intermittents. I'm very careful with landing the patches until I have a very high degree of confidence that the test failures are not caused by my patches, because I can imagine that backing out 40 patches is no fun.

I'm seeing test failures that I cannot reproduce locally (and often not even on the try server). In some cases I am able to come up with patches after an analysis of what could go wrong in theory, in other cases I'm stabbing in the dark. 

Here are some tests that I investigated in the past week (note: none of them are consistently reproducible, at least not locally, and often not on try servers either): 

(1) test_ext_cookies.html - timed out - a timeout of 100ms is apparently not enough to work around bug 1309637, so I'll increase the timeout.
(2) est_ext_i18n.html - timed out - I saw this on two try runs, but after adding debugging (logging) the test passed.
(3) test_ext_storage_content.html - multiple failures - most likely the same (but rarer) cause as https://reviewboard.mozilla.org/r/78670/diff/7#field_description
(4) test_ext_unload_frame.html - Unexpected reply: undefined - This test failure implies that the callback is still called after a frame was unloaded!
(5) test_ext_webnavigation.html - Got the expected 'manual_subframe' transitionType in the OnCommitted event - got "auto_subframe", expected "manual_subframe" - Given that webNavigation is still using the existing infrastructure, I don't even see how this could be caused by my changes.

Here is a try run from the patch set of your last review:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b79cc947c5d0

And the try run with fixed for (1) and (3), and logging for (2). All relevant tests PASS.

I'm going to run a few more try jobs, and if they are consistently green I will land the patches as-is.
Otherwise I'll conservatively set the shouldSynchronouslyUseParentAPI flag* to true to match the current behavior, and land a separate commit that flips the flag ~1 week later. So if there are any other timing issues caused by the asynchronous API, then intermittents will start showing up at the second patch

* shouldSynchronouslyUseParentAPI in ExtensionChild.jsm: https://reviewboard.mozilla.org/r/78672/diff/7#index_header

Comment 331

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/7-8/

Comment 332

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/7-8/

Comment 333

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/7-8/

Comment 334

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/7-8/

Comment 335

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/7-8/

Comment 336

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/7-8/

Comment 337

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/7-8/

Comment 338

[Rob Wu [:robwu]]

Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82418/diff/3-4/

Comment 339

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/7-8/

Comment 340

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/7-8/

Comment 341

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/7-8/

Comment 342

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/7-8/

Comment 343

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/7-8/

Comment 344

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/7-8/

Comment 345

[Rob Wu [:robwu]]

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77544/diff/6-7/

Comment 346

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/7-8/

Comment 347

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/8-9/

Comment 348

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/9-10/

Comment 349

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/8-9/

Comment 350

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/9-10/

Comment 351

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/8-9/

Comment 352

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/8-9/

Comment 353

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/7-8/

Comment 354

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/7-8/

Comment 355

[Rob Wu [:robwu]]

Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78662/diff/6-7/

Comment 356

[Rob Wu [:robwu]]

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78664/diff/6-7/

Comment 357

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/7-8/

Comment 358

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/7-8/

Comment 359

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/7-8/

Comment 360

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/7-8/

Comment 361

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/7-8/

Comment 362

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/7-8/

Comment 363

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/7-8/

Comment 364

[Rob Wu [:robwu]]

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79200/diff/6-7/

Comment 365

[Rob Wu [:robwu]]

Comment on attachment 8794528 [details]
Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80956/diff/4-5/

Comment 366

[Rob Wu [:robwu]]

Comment on attachment 8796950 [details]
Bug 1287007 - Remove contextMenusInternal.onClicked

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82600/diff/3-4/

Comment 367

[Rob Wu [:robwu]]

Comment on attachment 8796951 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_onUpdated.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82602/diff/3-4/

Comment 368

[Rob Wu [:robwu]]

Comment on attachment 8799980 [details]
Bug 1287007 - Adjust state test in browser_ext_windows_update.js on Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85018/diff/3-4/

Comment 369

[Rob Wu [:robwu]]

Comment on attachment 8800398 [details]
Bug 1287007 - Fix timing issue in browser_ext_contextMenus.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85304/diff/1-2/

Comment 370

[Rob Wu [:robwu]]

Comment on attachment 8800399 [details]
Bug 1287007 - Work-around for test failure in test_ext_cookies.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85306/diff/1-2/

Comment 371

[Rob Wu [:robwu]]

I ran all tests and the results look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52abdf3e6773 (there is one failing WebExtensions test, which is already tracked in bug 1292491).

I'm now running a subset of the tests (mochitests & xpcshell tests) ten times: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4e2bff19391
If there are no new unexpected failures I will land all patches.

Comment 372

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

(In reply to Rob Wu [:robwu] from comment #371)
> I ran all tests and the results look good:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=52abdf3e6773 (there
> is one failing WebExtensions test, which is already tracked in bug 1292491).

Given how consistent the failure is, I suspect you'll have to fix it before landing.

Comment 373

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

I can imagine why that test is failing.
http://searchfox.org/mozilla-central/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js#117-126

It's possible that, in between browser.windows.create finishing and adding the onUpdated listener, the tabs could have loaded. I think we should query the state of the tabs after we add the listener. If the URL is already correct, we can resolve the promise.

Comment 374

[Rob Wu [:robwu]]

Attachment: Bug 1287007 - Fix timing issue in browser_ext_windows_create_tabId.js

When tabs.onUpdated is called before the promise of windows.create
resolves, the test fails.
This test failure was observed on almost every try run with non-e10s,
and sometimes (rarely) with e10s.

Review commit: https://reviewboard.mozilla.org/r/87432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87432/

Comment 375

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

Comment on attachment 8803183 [details]
Bug 1287007 - Fix timing issue in browser_ext_windows_create_tabId.js

https://reviewboard.mozilla.org/r/87432/#review86434

Comment 376

[Rob Wu [:robwu]]

The try job that runs all tests 20 has been stuck at the same progress for two days (on OS X debug). There are no new failures on Linux debug/release, OS X release and Android debug/release: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea6bf867930b

I will run another try (in case something else landed in the past two days) and then land all patches.

Comment 377

[Rob Wu [:robwu]]

Comment on attachment 8788662 [details]
Bug 1287007 - Fix some flaws in ProxyAPIImplementation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77086/diff/8-9/

Comment 378

[Rob Wu [:robwu]]

Comment on attachment 8788663 [details]
Bug 1287007 - Improve errors for non-existing remote APIs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77054/diff/8-9/

Comment 379

[Rob Wu [:robwu]]

Comment on attachment 8788665 [details]
Bug 1287007 - Refactor test_ext_schemas_api_injection.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77058/diff/8-9/

Comment 380

[Rob Wu [:robwu]]

Comment on attachment 8788666 [details]
Bug 1287007 - Fix timing issue in browser/page action tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77060/diff/8-9/

Comment 381

[Rob Wu [:robwu]]

Comment on attachment 8788664 [details]
Bug 1287007 - Require "async" in schemas to match name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77056/diff/8-9/

Comment 382

[Rob Wu [:robwu]]

Comment on attachment 8788667 [details]
Bug 1287007 - Mark all browserAction methods as async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77062/diff/8-9/

Comment 383

[Rob Wu [:robwu]]

Comment on attachment 8788668 [details]
Bug 1287007 - Allow local implementations to call remote implementations

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77064/diff/8-9/

Comment 384

[Rob Wu [:robwu]]

Comment on attachment 8796629 [details]
Bug 1287007 - Derive context.principal from sandbox

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82418/diff/4-5/

Comment 385

[Rob Wu [:robwu]]

Comment on attachment 8788669 [details]
Bug 1287007 - Fix cross-process browser.storage.local serialization

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77066/diff/8-9/

Comment 386

[Rob Wu [:robwu]]

Comment on attachment 8788670 [details]
Bug 1287007 - Move ExtensionContext to separate file

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77068/diff/8-9/

Comment 387

[Rob Wu [:robwu]]

Comment on attachment 8788671 [details]
Bug 1287007 - Do not neuter error messages from the same principal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77070/diff/8-9/

Comment 388

[Rob Wu [:robwu]]

Comment on attachment 8788672 [details]
Bug 1287007 - Introduce ChildAPIManager to addon code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77072/diff/8-9/

Comment 389

[Rob Wu [:robwu]]

Comment on attachment 8788673 [details]
Bug 1287007 - Rename "context.type" to "context.viewType"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77074/diff/8-9/

Comment 390

[Rob Wu [:robwu]]

Comment on attachment 8788674 [details]
Bug 1287007 - Move background page API logic to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77076/diff/8-9/

Comment 391

[Rob Wu [:robwu]]

Comment on attachment 8789268 [details]
Bug 1287007 - Move extension.getViews to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77544/diff/7-8/

Comment 392

[Rob Wu [:robwu]]

Comment on attachment 8788675 [details]
Bug 1287007 - Move extension context initialization to ExtensionContent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77078/diff/8-9/

Comment 393

[Rob Wu [:robwu]]

Comment on attachment 8788676 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_getCurrent.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77080/diff/9-10/

Comment 394

[Rob Wu [:robwu]]

Comment on attachment 8788677 [details]
Bug 1287007 - Use IPC to share viewType, tabId and windowId

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77082/diff/10-11/

Comment 395

[Rob Wu [:robwu]]

Comment on attachment 8789269 [details]
Bug 1287007 - Fix test_ext_api_permissions.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77546/diff/9-10/

Comment 396

[Rob Wu [:robwu]]

Comment on attachment 8788678 [details]
Bug 1287007 - Use child's Extension instead of the process'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77084/diff/10-11/

Comment 397

[Rob Wu [:robwu]]

Comment on attachment 8789270 [details]
Bug 1287007 - Remove dead code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77548/diff/9-10/

Comment 398

[Rob Wu [:robwu]]

Comment on attachment 8789271 [details]
Bug 1287007 - Set the principal in ChildAPIManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77550/diff/9-10/

Comment 399

[Rob Wu [:robwu]]

Comment on attachment 8789674 [details]
Bug 1287007 - Only close extension tabs upon shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77810/diff/8-9/

Comment 400

[Rob Wu [:robwu]]

Comment on attachment 8789675 [details]
Bug 1287007 - Use frame script to initialize background/popup page

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77812/diff/8-9/

Comment 401

[Rob Wu [:robwu]]

Comment on attachment 8791152 [details]
Bug 1287007 - Track message manager / browser swaps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78662/diff/7-8/

Comment 402

[Rob Wu [:robwu]]

Comment on attachment 8791153 [details]
Bug 1287007 - Set parent cloneScope to child cloneScope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78664/diff/7-8/

Comment 403

[Rob Wu [:robwu]]

Comment on attachment 8791154 [details]
Bug 1287007 - Move part of browserAction and pageAction to child

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78666/diff/8-9/

Comment 404

[Rob Wu [:robwu]]

Comment on attachment 8791155 [details]
Bug 1287007 - Fix "onclick" in contextMenus, to child.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78668/diff/8-9/

Comment 405

[Rob Wu [:robwu]]

Comment on attachment 8791156 [details]
Bug 1287007 - Fix timing issue in test_ext_storage.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78670/diff/8-9/

Comment 406

[Rob Wu [:robwu]]

Comment on attachment 8791157 [details]
Bug 1287007 - Enable proxying of most APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78672/diff/8-9/

Comment 407

[Rob Wu [:robwu]]

Comment on attachment 8791158 [details]
Bug 1287007 - Fix test_ext_management_uninstall_self.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78674/diff/8-9/

Comment 408

[Rob Wu [:robwu]]

Comment on attachment 8791159 [details]
Bug 1287007 - Remove .contentWindow from ProxyContext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78676/diff/8-9/

Comment 409

[Rob Wu [:robwu]]

Comment on attachment 8791160 [details]
Bug 1287007 - Make window.close in extension pages async

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78678/diff/8-9/

Comment 410

[Rob Wu [:robwu]]

Comment on attachment 8791930 [details]
Bug 1287007 - Make browser_ext_tabs_executeScript.js reliable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79200/diff/7-8/

Comment 411

[Rob Wu [:robwu]]

Comment on attachment 8794528 [details]
Bug 1287007 - Fix timing issue in test_delay_update_webextension.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80956/diff/5-6/

Comment 412

[Rob Wu [:robwu]]

Comment on attachment 8796950 [details]
Bug 1287007 - Remove contextMenusInternal.onClicked

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82600/diff/4-5/

Comment 413

[Rob Wu [:robwu]]

Comment on attachment 8796951 [details]
Bug 1287007 - Fix timing issue in browser_ext_tabs_onUpdated.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82602/diff/4-5/

Comment 414

[Rob Wu [:robwu]]

Comment on attachment 8799980 [details]
Bug 1287007 - Adjust state test in browser_ext_windows_update.js on Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85018/diff/4-5/

Comment 415

[Rob Wu [:robwu]]

Comment on attachment 8800398 [details]
Bug 1287007 - Fix timing issue in browser_ext_contextMenus.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85304/diff/2-3/

Comment 416

[Rob Wu [:robwu]]

Comment on attachment 8800399 [details]
Bug 1287007 - Work-around for test failure in test_ext_cookies.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85306/diff/2-3/

Comment 417

[Rob Wu [:robwu]]

Comment on attachment 8803183 [details]
Bug 1287007 - Fix timing issue in browser_ext_windows_create_tabId.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87432/diff/1-2/

Comment 418

[Rob Wu [:robwu]]

Rebased due to merge conflicts. Before landing, I'll run the tests 3x (just in case).

Comment 419

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e6462c7563d
Fix some flaws in ProxyAPIImplementation r=billm
https://hg.mozilla.org/integration/autoland/rev/fbbeab9bd95b
Improve errors for non-existing remote APIs r=billm
https://hg.mozilla.org/integration/autoland/rev/b91a39e959ff
Refactor test_ext_schemas_api_injection.js r=billm
https://hg.mozilla.org/integration/autoland/rev/930d8616be95
Fix timing issue in browser/page action tests r=billm
https://hg.mozilla.org/integration/autoland/rev/1edf5fa29a43
Require "async" in schemas to match name r=billm
https://hg.mozilla.org/integration/autoland/rev/7292a34a169e
Mark all browserAction methods as async r=billm
https://hg.mozilla.org/integration/autoland/rev/3ca8f3e64eeb
Allow local implementations to call remote implementations r=billm
https://hg.mozilla.org/integration/autoland/rev/32d29dd44c65
Derive context.principal from sandbox r=billm,kmag
https://hg.mozilla.org/integration/autoland/rev/bfb7562c2406
Fix cross-process browser.storage.local serialization r=billm
https://hg.mozilla.org/integration/autoland/rev/5df860aa9c9c
Move ExtensionContext to separate file r=billm
https://hg.mozilla.org/integration/autoland/rev/d6ac05fb1b56
Do not neuter error messages from the same principal r=billm
https://hg.mozilla.org/integration/autoland/rev/642f1b1afa90
Introduce ChildAPIManager to addon code r=billm
https://hg.mozilla.org/integration/autoland/rev/cb7fa050a6bf
Rename "context.type" to "context.viewType" r=billm
https://hg.mozilla.org/integration/autoland/rev/1ed5b81ab606
Move background page API logic to child r=billm
https://hg.mozilla.org/integration/autoland/rev/f86e5930ef49
Move extension.getViews to child r=billm
https://hg.mozilla.org/integration/autoland/rev/9746d5da8a51
Move extension context initialization to ExtensionContent r=billm
https://hg.mozilla.org/integration/autoland/rev/3d18af0cc526
Fix timing issue in browser_ext_tabs_getCurrent.js r=billm
https://hg.mozilla.org/integration/autoland/rev/6b9a398be651
Use IPC to share viewType, tabId and windowId r=billm
https://hg.mozilla.org/integration/autoland/rev/e3514b399ec4
Fix test_ext_api_permissions.js r=billm
https://hg.mozilla.org/integration/autoland/rev/f9da44ff592d
Use child's Extension instead of the process' r=billm
https://hg.mozilla.org/integration/autoland/rev/37414d1fd5ef
Remove dead code r=billm
https://hg.mozilla.org/integration/autoland/rev/817299c12d6d
Set the principal in ChildAPIManager r=billm
https://hg.mozilla.org/integration/autoland/rev/fca5d1ec8c26
Only close extension tabs upon shutdown r=billm
https://hg.mozilla.org/integration/autoland/rev/709a07bf0362
Use frame script to initialize background/popup page r=billm
https://hg.mozilla.org/integration/autoland/rev/54c252512468
Track message manager / browser swaps r=billm
https://hg.mozilla.org/integration/autoland/rev/09fa6078095c
Set parent cloneScope to child cloneScope r=billm
https://hg.mozilla.org/integration/autoland/rev/4c43afedf60f
Move part of browserAction and pageAction to child r=billm
https://hg.mozilla.org/integration/autoland/rev/f2f75e6e2d60
Fix "onclick" in contextMenus, to child. r=billm
https://hg.mozilla.org/integration/autoland/rev/44c68913ebf5
Fix timing issue in test_ext_storage.js r=billm
https://hg.mozilla.org/integration/autoland/rev/851e08a1f368
Enable proxying of most APIs. r=billm
https://hg.mozilla.org/integration/autoland/rev/b07b3ad62e31
Fix test_ext_management_uninstall_self.js r=bsilverberg
https://hg.mozilla.org/integration/autoland/rev/919c8b9e2ab4
Remove .contentWindow from ProxyContext r=billm
https://hg.mozilla.org/integration/autoland/rev/437ef72ca354
Make window.close in extension pages async r=billm
https://hg.mozilla.org/integration/autoland/rev/2b6e6f661409
Make browser_ext_tabs_executeScript.js reliable r=billm
https://hg.mozilla.org/integration/autoland/rev/daf166cb192a
Fix timing issue in test_delay_update_webextension.js r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/e6a612e71387
Remove contextMenusInternal.onClicked r=billm
https://hg.mozilla.org/integration/autoland/rev/3915db2d27b1
Fix timing issue in browser_ext_tabs_onUpdated.js r=billm
https://hg.mozilla.org/integration/autoland/rev/424e3fa0b282
Adjust state test in browser_ext_windows_update.js on Linux r=kmag
https://hg.mozilla.org/integration/autoland/rev/434598e8b200
Fix timing issue in browser_ext_contextMenus.js r=billm
https://hg.mozilla.org/integration/autoland/rev/f10221d238ac
Work-around for test failure in test_ext_cookies.html r=billm
https://hg.mozilla.org/integration/autoland/rev/c0a8fecacb46
Fix timing issue in browser_ext_windows_create_tabId.js r=billm

Comment 420

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e6462c7563d
https://hg.mozilla.org/mozilla-central/rev/fbbeab9bd95b
https://hg.mozilla.org/mozilla-central/rev/b91a39e959ff
https://hg.mozilla.org/mozilla-central/rev/930d8616be95
https://hg.mozilla.org/mozilla-central/rev/1edf5fa29a43
https://hg.mozilla.org/mozilla-central/rev/7292a34a169e
https://hg.mozilla.org/mozilla-central/rev/3ca8f3e64eeb
https://hg.mozilla.org/mozilla-central/rev/32d29dd44c65
https://hg.mozilla.org/mozilla-central/rev/bfb7562c2406
https://hg.mozilla.org/mozilla-central/rev/5df860aa9c9c
https://hg.mozilla.org/mozilla-central/rev/d6ac05fb1b56
https://hg.mozilla.org/mozilla-central/rev/642f1b1afa90
https://hg.mozilla.org/mozilla-central/rev/cb7fa050a6bf
https://hg.mozilla.org/mozilla-central/rev/1ed5b81ab606
https://hg.mozilla.org/mozilla-central/rev/f86e5930ef49
https://hg.mozilla.org/mozilla-central/rev/9746d5da8a51
https://hg.mozilla.org/mozilla-central/rev/3d18af0cc526
https://hg.mozilla.org/mozilla-central/rev/6b9a398be651
https://hg.mozilla.org/mozilla-central/rev/e3514b399ec4
https://hg.mozilla.org/mozilla-central/rev/f9da44ff592d
https://hg.mozilla.org/mozilla-central/rev/37414d1fd5ef
https://hg.mozilla.org/mozilla-central/rev/817299c12d6d
https://hg.mozilla.org/mozilla-central/rev/fca5d1ec8c26
https://hg.mozilla.org/mozilla-central/rev/709a07bf0362
https://hg.mozilla.org/mozilla-central/rev/54c252512468
https://hg.mozilla.org/mozilla-central/rev/09fa6078095c
https://hg.mozilla.org/mozilla-central/rev/4c43afedf60f
https://hg.mozilla.org/mozilla-central/rev/f2f75e6e2d60
https://hg.mozilla.org/mozilla-central/rev/44c68913ebf5
https://hg.mozilla.org/mozilla-central/rev/851e08a1f368
https://hg.mozilla.org/mozilla-central/rev/b07b3ad62e31
https://hg.mozilla.org/mozilla-central/rev/919c8b9e2ab4
https://hg.mozilla.org/mozilla-central/rev/437ef72ca354
https://hg.mozilla.org/mozilla-central/rev/2b6e6f661409
https://hg.mozilla.org/mozilla-central/rev/daf166cb192a
https://hg.mozilla.org/mozilla-central/rev/e6a612e71387
https://hg.mozilla.org/mozilla-central/rev/3915db2d27b1
https://hg.mozilla.org/mozilla-central/rev/424e3fa0b282
https://hg.mozilla.org/mozilla-central/rev/434598e8b200
https://hg.mozilla.org/mozilla-central/rev/f10221d238ac
https://hg.mozilla.org/mozilla-central/rev/c0a8fecacb46

Comment 421

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

\o/!!!