1417639 - toolkit chrome.ini tests run only in-process with e10s disabled

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

Bug 1416872 wasn't caught due to this, we should either make chrome.ini tests run both ways, or move those tests to mochitest-common.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1417639 fix background page alert and test,

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

Primary items changed:

- The console monitoring in SimpleTest would not work in mochitest-plain.  I've created a new chrome_console.js that is used.  It is simpler than the SimpleTest version since it only covers our use case.
- alert overriding for background page is partly fixed, still disabled for android, and disabled for remote where opening the browser console fails.
- most tests not moved use window.open and operate on the returned window.  Those will need a second round.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

https://reviewboard.mozilla.org/r/201862/#review207526

::: toolkit/components/extensions/ext-c-toolkit.js:17
(Diff revision 2)
> -global.initializeBackgroundPage = (contentWindow) => {
> +global.initializeBackgroundPage = (contentWindow, uri) => {
>    // Override the `alert()` method inside background windows;
>    // we alias it to console.log().
>    // See: https://bugzilla.mozilla.org/show_bug.cgi?id=1203394
>    let alertDisplayedWarning = false;
>    let alertOverwrite = text => {
>      if (!alertDisplayedWarning) {
>        DevToolsShim.openBrowserConsole();
> -      contentWindow.console.warn("alert() is not supported in background windows; please use console.log instead.");
> +      let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
> +      consoleMsg.init("alert() is not supported in background windows; please use console.log instead.",
> +                      uri.prePath, null, 0, 0, Ci.nsIScriptError.warningFlag, "webextension");
> +      Services.console.logMessage(consoleMsg);
>  
>        alertDisplayedWarning = true;
>      }
>  
> -    contentWindow.console.log(text);
> +    Services.console.logStringMessage(text);
>    };
>    Cu.exportFunction(alertOverwrite, contentWindow, {defineAs: "alert"});

Please move this to a separate patch.

::: toolkit/components/extensions/test/mochitest/chrome_console.js:48
(Diff revision 2)
> +  Services.console.registerListener(listener);
> +}
> +
> +addMessageListener("consoleStart", messages => {
> +  for (let msg of messages) {
> +    // Something happens to RegExp when receiving in the chrome script.

What do you mean "something happens"? You should wind up with a RegExp object from a different compartment, but it should still work as a RegExp.

::: toolkit/components/extensions/test/mochitest/head_cookies.js:94
(Diff revision 2)
> +  let stepOne = SpecialPowers.loadChromeScript(() => {
> +    const {Services} = Components.utils.import("resource://gre/modules/Services.jsm", {});

There's a lot of boilerplate in each of these chrome scripts. Can you please add a helper to hide most of it?

::: toolkit/components/extensions/test/mochitest/head_cookies.js:151
(Diff revision 2)
> +    function is(a, b, message) {
> +      sendAsyncMessage("is", {a, b, message});
> +    }

Unnecessary. Just use `assert.equal`

::: toolkit/components/extensions/test/mochitest/test_ext_background_page.html:19
(Diff revision 2)
> -Cu.import("resource://testing-common/TestUtils.jsm");
> -
>  /* eslint-disable mozilla/balanced-listeners */
>  
>  add_task(async function testAlertNotShownInBackgroundWindow() {
> -  ok(!Services.wm.getEnumerator("alert:alert").hasMoreElements(),
> +  ok(!SpecialPowers.Services.wm.getEnumerator("alert:alert").hasMoreElements(),

This won't work in the child process.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

https://reviewboard.mozilla.org/r/201862/#review207526

> What do you mean "something happens"? You should wind up with a RegExp object from a different compartment, but it should still work as a RegExp.

An object is received, but it is not "instanceof RegExp".  I thought about doing some other validation, but it was just as easy to re-RegExp it.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1417639 move tests out of mochitest-chrome,

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

https://reviewboard.mozilla.org/r/201860/#review209048

Hi Shane,
I took a look at this patches, but I focused my attention on the pieces related to the `background page alert` in this first pass, and so the following comments are mostly related to issues related to that.

(nevertheless I'm going to take a deeper look also to the rest of these two patches and add more review comments about them).

::: toolkit/components/extensions/ext-c-toolkit.js:17
(Diff revision 3)
>  /* exported EventManager */
>  /* global EventManager: false */
>  
>  global.EventManager = ExtensionCommon.EventManager;
>  
> -global.initializeBackgroundPage = (contentWindow) => {
> +global.initializeBackgroundPage = (contentWindow, uri) => {

`uri` is not needed if we use the information that we can retrieve from `Components.stack.caller` as described in more detail in one of the review comments below.

::: toolkit/components/extensions/ext-c-toolkit.js:24
(Diff revision 3)
>    // we alias it to console.log().
>    // See: https://bugzilla.mozilla.org/show_bug.cgi?id=1203394
>    let alertDisplayedWarning = false;
>    let alertOverwrite = text => {
>      if (!alertDisplayedWarning) {
>        DevToolsShim.openBrowserConsole();

This is not going to work when the extension is running in the child extension process, this part should be done in the main process (and I guess this is the reason why the test has not been moved into mochitest-common.ini).

One one to achieve it could be to pass the `context` instance instead of just its contentWindow to this `initializeBackgroundPage` function from `ExtensionPageChild.jsm`:

```
defineLazyGetter(ExtensionPageContextChild.prototype, "childManager", function() {
  ...
  
  if (this.viewType == "background") {
    apiManager.global.initializeBackgroundPage(this);
  }

  ...
});

```

and then ask to the main process to open the BrowserConsole using something like `context.childManager.callParentAsyncFunction("runtime.openBrowserConsole", []);`,
where "runtime.openBrowserConsole" would be an API method defined in "ext-runtime.js" but not exposed to the extension code from the schema (it is basically a private function only available to the internals and not declared in the WE json API schema files),
which would open the BrowserConsole from the main process (but only if the platform is not android, otherwise it doesn't do anything, and DevToolsShim is a lazy getter and so it will not be harmful if we don't try to access DevToolsShim from Firefox for Android), e.g. in "ext-runtime.js":

```
        openOptionsPage: function() {
          ...
        },  

        // This function is not exposed to the extension js code and it is only used
        // by the alert function redefined into the background pages to be able to
        // open the BrowserConsole from the main process.
        openBrowserConsole() {
          if (AppConstants.platform !== "android") {
            DevToolsShim.openBrowserConsole();
          }
        },

```

::: toolkit/components/extensions/ext-c-toolkit.js:25
(Diff revision 3)
>    // See: https://bugzilla.mozilla.org/show_bug.cgi?id=1203394
>    let alertDisplayedWarning = false;
>    let alertOverwrite = text => {
>      if (!alertDisplayedWarning) {
>        DevToolsShim.openBrowserConsole();
> -      contentWindow.console.warn("alert() is not supported in background windows; please use console.log instead.");
> +      let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);

I think that it is ok to use the console service here, but I would prefer if we could ensure that these messages are going to be visible in the addon debugger as well and to associate them to the actual caller filename, lineNumber and columnNumber instead of the extension base url.

To achieve it we can remove the "uri" parameter from the `initializeBackgroundPage` page function and use `Components.stack.caller` to get the info related to actual caller, and `getInnerWindowID` (which can be retrieved from ExtensionUtils) to officailly associate the message to the background page window id, something like:

```
  const innerWindowID = getInnerWindowID(context.contentWindow);

  function logWarningMessage({text, filename, lineNumber, columnNumber}) {
    let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
    consoleMsg.initWithWindowID(text, filename, null, lineNumber, columnNumber,
                                Ci.nsIScriptError.warningFlag, "webextension",
                                innerWindowID);
    Services.console.logMessage(consoleMsg);
  }

  let alertOverwrite = text => {
    const {
      filename, columnNumber, lineNumber,
    } = Components.stack.caller;
  
        if (!alertDisplayedWarning) {
      context.childManager.callParentAsyncFunction("runtime.openBrowserConsole", []);

      logWarningMessage({
        text: "alert() is not supported in background windows; please use console.log instead.",
        filename, lineNumber, columnNumber,
      });

      alertDisplayedWarning = true;
    }

    logWarningMessage({text, filename, lineNumber, columnNumber});
  };

}
```

I'd suggest to use a warning message for both the messages because currently, mainly because nsIScriptError instances with the Ci.nsIScriptError.infoFlag flag are not currently visible in the Addon Debugger webconsole (and we want both these messages to be visible in the Addon Debugging webconsole with the correct filename, lineNumber and columnNumber, so that the developer can more easily find where it should apply a fix to remove these warning messages).

To be fair, I think that replacing the alert by opening the BrowserConsole has not been a great choice from a user point of view, I would prefer if we could send the "please don't use alert" warning message as described above (because it is a message for the developer), but to turn the text of the alert call into a notification (instead of opening the BrowserConsole, because the target of the text is likely to be the user, and I don't think that we should ever open for a user, especially not just for an extension that is still using an alert in its background page).

Another upside of the notification would be that we could probably make it to work also on Firefox for Android.

Anyway, I'm totally ok on fixing the breakage of this implementation by still log both these messages and open the Browser Console as a first step, and discuss about changing it in a follow up.

As an additional side note (related to Comment:

the reason why `contentWindow.console.*` is not caught by the console monitor is that the console service and the console API are two different kind of things and by monitoring the console service we are not listening for the messages logged using the console API.

Also, when running in oop mode, the console API messages are in a different process and they are sent to the Browser Console through the forwarding implemented by Bug 1382968 over the Remote Debugging protocol (and not through the main process console service).

And so to be able to monitor console API messages related to an extension we have to connect a debugger client to the extension, attach the console actor and listen for the ConsoleAPI events received on the debugger client.

::: toolkit/components/extensions/test/mochitest/chrome_console.js:32
(Diff revision 3)
> +  let counter = 0;
> +  function listener(msg) {
> +    if (msg.message === "SENTINEL" && !msg.isScriptError) {
> +      sendAsyncMessage("consoleDone", {
> +        ok: forbidUnexpectedMsgs ? counter == msgs.length : counter >= msgs.length,
> +        message: `monitorConsole | messages left expected ${counter} got ${msgs.length}`,

If I'm reading this correctly, `counter` and `msgs.length` are in the wrong order here:

`counter` seems to be the number of messages that we've got and `msgs.length` the number of messages that we expected.

Also, `forbitUnexpectedMsgs` is going to be always `undefined` because is not being passed to `monitorConsole` from the `monitorConsole(messages)` at line 56, and it is not received by the "consoleStart" message listener because it is not a parameter in `consoleMonitor.start` as it is currently defined in the head.js at line 71 and 74.

By looking at the test output it seems that we currently caught 3 console messages instead of the 2 expected because from the `assert` injected by SpecialPowers in the chrome script context there is a `"Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "chrome://specialpowers/content/SpecialPowersObserverAPI.js" line: 501}]` warning message emitted (which seems to be not due to what we are passing to the `assert.ok` function, because we are only passing a boolean and the string for the assertion message).

This warning doesn't seem to prevent the assertion from working correctly, and it would not make the test to fail (at least not while `forbitUnexpectedMsgs` is not `true`), but I thought that it was worth to mention it in the review comments.

::: toolkit/components/extensions/test/mochitest/mochitest.ini:4
(Diff revision 3)
>  [DEFAULT]
>  tags = webextensions in-process-webextensions
>  
> +[test_ext_background_page.html]

the `head.js` file and the `chrome_cleanup_script.js` and `chrome_console.js` are only defined in the `mochitest-common.ini` file and so they will not be available for these two tests listed here (and I looked at `test_ext_background_page.html` and it definitely needs all the three support files mentioned above) and so the test will currently fail.

Anyway I think that we can make this to work in OOP mode like I described in the review comments above, and in that case we can just move it into `mochitest-common.ini`.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

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

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

I moved some of the stuff around between patches just in case we want to uplift the alert fix.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

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

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

https://reviewboard.mozilla.org/r/201862/#review211900

::: toolkit/components/extensions/test/mochitest/chrome_console.js:32
(Diff revision 5)
> +  let counter = 0;
> +  function listener(msg) {
> +    if (msg.message === "SENTINEL" && !msg.isScriptError) {
> +      sendAsyncMessage("consoleDone", {
> +        ok: counter >= msgs.length,
> +        message: `monitorConsole | messages left expected ${counter} got ${msgs.length}`,

This should be `expected ${msgs.length} got ${counter}`

::: toolkit/components/extensions/test/mochitest/head.js:4
(Diff revision 5)
>  "use strict";
>  
>  /* exported AppConstants, Assert */
> +/* global addMessageListener, sendAsyncMessage, assert */

`addMessageListener`, `sendAsyncMessage` and `assert` should only be used inside the loaded chrome scripts, and by defining it here eslintrc will not be able to raise validation errors when they should not be used.

I would prefer if we can avoid to add them here in the head.js, e.g. using one of the following alternative approaches:

- using `const {addMessageListener, ...} = this`
  at the beginning of the chrome scripts defined in a test file should be able to retrieve these functions and not be detected by eslintrc as a linting error
  
- add the eslint comment only on the tests that are actually using it (which would still have the issue described above, but at least it would be restricted to the files that have the eslint comment explicitly in their preamble)

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

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

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

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

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

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

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

https://reviewboard.mozilla.org/r/201862/#review212270

r+ (with the small tweak suggested on test_ext_background_page.html)

::: toolkit/components/extensions/test/mochitest/mochitest_console.js:5
(Diff revision 7)
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +const {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
> +const {addMessageListener, sendAsyncMessage} = this;

I wasn't worried about the eslint ```/* global ... */``` comment in this file, because this entire file runs as a chrome script and so it makes sense to use the eslint global comment to whilelist the two globals.

Nevertheless, it is also fine to use the `const {addMessageListener, ...} = this` here and avoit it.

::: toolkit/components/extensions/test/mochitest/test_ext_background_page.html:31
(Diff revision 7)
>        browser.test.notifyPass("alertCalled");
>      },
>    });
>  
> -  await extension.startup();
> -
> +  let consoleOpened = loadChromeScript(() => {
> +    assert.ok(!Services.wm.getEnumerator("alert:alert").hasMoreElements(),

What I meant in my previous comment is that the 

```
/* global sendAsyncMessage, assert */
```

affects the entire file eslint validation, but these globals are only going to be available inside the function passed to `loadChromeScript` as a parameter.

And so, I would prefer to add:

```
const {sendAsyncMessage, assert} = this
```

here and at line 48 and line 59, and remove the eslint comment at line 17.

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

https://reviewboard.mozilla.org/r/202246/#review212282

Thanks Shane, this conversion from mochitest-chrome to mochitest-plain was very much needed!

I've added some additional review comments related to a few nits (mostly about some additional ```/* global ... */``` eslint comment that we can remove) but besides that it looks great.

r+ (with the some minor tweaks applied)

::: toolkit/components/extensions/test/mochitest/chrome.ini:13
(Diff revision 6)
>    file_sample.html
>    file_with_images.html
>    webrequest_chromeworker.js
>    webrequest_test.jsm
>  tags = webextensions in-process-webextensions
>  

Nice to have:

I think that a "WARNING" comment here could be useful as a reminder that we should not add a new test file here if there are very good reasons to (and that if we really need to, a new bugzilla issue should also be filed, to explain the reasons and how /when we can move the file into a test suite that runs in e10s mode, and link it to the code debts issue on bugzilla).

::: toolkit/components/extensions/test/mochitest/mochitest.ini:5
(Diff revision 6)
>  [DEFAULT]
>  tags = webextensions in-process-webextensions
>  
> +[test_ext_storage_cleanup.html]
> +# storage_cleanup: clearing localStorage fails with oop

It looks that we should add this test in the list of the code debts that we want to fix in a follow up.

::: toolkit/components/extensions/test/mochitest/test_ext_idle.html:51
(Diff revision 6)
>    });
>  
>    await extension.startup();
> +
> +  let chromeScript = loadChromeScript(async () => {
> +    const idleService = Cc["@mozilla.org/widget/idleservice;1"].getService(Ci.nsIIdleService);

we can add ```const {sendAsyncMessage} = this;``` here and remove the ```/* global ... */``` eslint comment at line 16.

::: toolkit/components/extensions/test/xpcshell/test_ext_trustworthy_origin.js:7
(Diff revision 6)
> -
>  /**
>   * This test is asserting that moz-extension: URLs are recognized as trustworthy local origins
>   */
>  
> -Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +add_task(function test_isOriginPotentiallyTrustworthnsIContentSecurityManagery() {

Nit, this is a pretty long name :-) (anyway, it is not a big deal)

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_mozextension.html:104
(Diff revision 6)
>    // Use webrequest to monitor moz-extension:// requests
>    let extension = ExtensionTestUtils.loadExtension({
>      manifest: {
>        permissions: [
>          "webRequest",
> +        "webRequestBlocking",

Is the "webRequestBlocking" permission needed? it doesn't seem to be used by the background page of the related test extension.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_expiry.html:50
(Diff revision 6)
>      },
>      background,
>    });
>  
> -  let cookieSvc = SpecialPowers.Services.cookies;
> -
> +  let chromeScript = loadChromeScript(() => {
> +    Services.cookies.add(".example.com", "/", "first", "one", false, false, false, Date.now() / 1000 + 1);

we can add ```const {sendAsyncMessage} = this;``` here and at line 60, and remove the ```/* global ... */``` eslint comment at line 17.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

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

Comment 24

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2393684a70c518a4fed1d808d43343bfd80c2efe

Comment 25

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f691232b20ca27ee8542041e90db15824e6f118

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8930779 [details]
Bug 1417639 fix background page alert and test,

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

Comment 27

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8931159 [details]
Bug 1417639 move tests out of mochitest-chrome,

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e1f415d09e8
fix background page alert and test, r=rpl
https://hg.mozilla.org/integration/autoland/rev/bf64f49f19aa
move tests out of mochitest-chrome, r=rpl

Comment 29

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/6e1f415d09e8
https://hg.mozilla.org/mozilla-central/rev/bf64f49f19aa

Comment 30

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.