1190663 - Implement window.close() for browser action popup HTML

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Comment 1

[Mindaugas <LA-MJ>]

Also affects pageAction popup.html:

> Scripts may not close windows that were not opened by script.

Comment 2

[Kris Maglione [:kmag]]

This should have been fixed by bug 1217129, but it still needs tests.

Comment 3

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl

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

Comment 4

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

Comment on attachment 8724493 [details]
MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl

https://reviewboard.mozilla.org/r/37047/#review33677

If I'm not wrong, this updated test case is supposed to fail if the popup is not closed by the "window.close()" called from inside the popup extension page.

I tried to comment out the "window.close()" to ensure that the test fails when it should, but all the test assertions passes successfully even when that line is commented out,
and so I tried to find where the popup is being closed before the "window.close()" has been called.

Follows some notes about the lines that seem to be related the issue described above:

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:135
(Diff revision 1)
> -    yield closeBrowserAction(extension);
> +      yield closeBrowserAction(extension);

This line seems to close the browserAction's popup immediatelly after the test case which opens the popup, and so the popup is never closed by the "window.close()".

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:143
(Diff revision 1)
>        panel.hidePopup();

Same here, but for the pageAction popups:

This line seems to always hide the popup after the test case which opens it, and so the popup is never closed by the "window.close()" as supposed by this updated version of the test case.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8724493 [details]
MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl

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

Comment 6

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/37047/#review33677

Good catch. I should have checked that the tests failed without calling close().

Comment 7

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

Comment on attachment 8724493 [details]
MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl

https://reviewboard.mozilla.org/r/37047/#review33919

Besides a small nit, the "browser_ext_browserAction_popup.js" test file logs an error when it runs twice in a row (which, even if the test passes when it happens, is probably something worth to check)

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:5
(Diff revisions 1 - 2)
>  function* testInArea(area) {

If we try to run "browser_ext_browserAction_popup.js" twice in a row (e.g. by running it with "./mach mochitest browser/components/extensions/test/browser/browser_ext_browserAction_popup.js --repeat 2"), the following error is logged:

JavaScript error: chrome://browser/content/ext-utils.js, line 303: TypeError: this.browser is null

The error is raised from the popup method |resizeBrowser|, which has been called at the start of the second run of the test case (and this.browser is null).

On the contrary, this doesn't happen if we try to run with "--repeat 2" the pageAction popup tests.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:17
(Diff revisions 1 - 2)
>        "popup-a.html": `<script src="popup-a.js"></script>`,

Nit: in the browserAction popup test cases, we should define the extension html pages using the same technique used in the pageAction popup tests:

      let scriptPage = url => \\\`<html><head><meta charset="utf-8"><script src="$\\\{url\\\}"></script></head></html>\\\`;
      ...
      
      files: \\\{
        "pupup-a.html": scriptPage("popup-a.js"),
        ...
        
to reduce noise related to the logged console messages related to the missing page charset definition.

Comment 8

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/37047/#review33919

> If we try to run "browser_ext_browserAction_popup.js" twice in a row (e.g. by running it with "./mach mochitest browser/components/extensions/test/browser/browser_ext_browserAction_popup.js --repeat 2"), the following error is logged:
> 
> JavaScript error: chrome://browser/content/ext-utils.js, line 303: TypeError: this.browser is null
> 
> The error is raised from the popup method |resizeBrowser|, which has been called at the start of the second run of the test case (and this.browser is null).
> 
> On the contrary, this doesn't happen if we try to run with "--repeat 2" the pageAction popup tests.

That error isn't new. It happens because, in those cases, we close the popup so clickly that the callback to resize the browser happens after it's already closed. It's not exactly ideal, but it's also not really an issue.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8724493 [details]
MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl

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

Comment 10

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

Comment on attachment 8724493 [details]
MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl

https://reviewboard.mozilla.org/r/37047/#review34145

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/09d8e3caf51daf1554673799609b575de4383a02
Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r=rpl

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/38d23f0168980b27bb88bf20a8cba7f907bc495e
Backed out changeset 09d8e3caf51d (bug 1190663) for intermittent mochitest failures on Windows

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/afa990f7034a12b78e1cba02b532a254c2d1990f
Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r=rpl

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afa990f7034a

Comment 15

[Will Bamberg [:wbamberg]]

I've added a bit on using window.close() in the intro docs for browser actions: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Anatomy_of_a_WebExtension#Browser_actions_2