1253129 - Support focused=false in the browser.windows.create

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Kris Maglione [:kmag]]

Chrome creates these windows with the normal stacking order and positioning, but does not focus them.

Comment 2

[Wladimir Palant]

It would actually help already if the windows.create() call wouldn't error out when seeing focused=true. The Chrome documentation (and current MDN documentation since it's identical) doesn't really make it clear that creating a focused window is the default behavior. So extensions will often specify focused=true explicitly and fail in Firefox for no good reason whatsoever.

Comment 3

[Will Bamberg [:wbamberg]]

(In reply to Wladimir Palant from comment #2)
> It would actually help already if the windows.create() call wouldn't error
> out when seeing focused=true. The Chrome documentation (and current MDN
> documentation since it's identical) doesn't really make it clear that
> creating a focused window is the default behavior. So extensions will often
> specify focused=true explicitly and fail in Firefox for no good reason
> whatsoever.

Thanks for pointing this out. I've updated the docs page.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Wladimir Palant from comment #2)
> It would actually help already if the windows.create() call wouldn't error
> out when seeing focused=true. The Chrome documentation (and current MDN
> documentation since it's identical) doesn't really make it clear that
> creating a focused window is the default behavior. So extensions will often
> specify focused=true explicitly and fail in Firefox for no good reason
> whatsoever.

Currently, we don't guarantee either that the new window receives focus or that it doesn't. In general, it does receive focus, but it's mostly up to the OS, and there are corner cases where it doesn't.

Comment 5

[Tomislav Jovanovic :zombie]

I tried to find, but it looks like we don't have platform support to "unfocus" a window, and that's the problem here, right?

But even with that, we might as well allow that option (and only do a focus if set to true), as that is what we do for windows.update() method.

http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-windows.js#197

Comment 6

[Robert AB]

(seniori - Tab Session Manager developer):

There is no API to create a window with a focus
https://bugzilla.mozilla.org/show_bug.cgi?id=1253129

Source:
https://github.com/sienori/Tab-Session-Manager/issues/375#issuecomment-480531258

Comment 7

[Rob Wu [:robwu]]

From a test of Chromium on Linux (KDE), when I have three browser windows, ONE, TWO and THREE (in that focus order), calling chrome.windows.create({focused: false}) has the following behavior:

• creates an unfocused window (FOUR);
• the window (FOUR) is rendered behind the current active window (but in front of other non-focused windows), i.e. FOUR is between TWO and THREE.
• when pressing Alt-Tab, the active window (THREE) switches to TWO. When pressing Alt-Tab again, the focus changes from TWO to FOUR.

The last point is not really intuitive. I wouldn't put that much weight in what Chrome does, as long as there is some sane behavior, i.e. focused=false doesn't change the active window.

Comment 8

[Rob Wu [:robwu]]

Is it possible to open a browser window in the background (without it being focused)?

If that's not possible, then I suppose that we could save the active window before opening a new window and then focus it again after opening the new window. It's not pretty (and may steal focus if Firefox is not focused to start with), but it helps extensions with porting their extensions.

Comment 9

[Shane Caraveo (:mixedpuppy)]

the window (FOUR) is rendered behind the current active window

That is a behavior (popunder) I'm not certain we want to support. If I understand the issue, we're giving an error when focus is used in window.create. What if we just ignore it instead? Do we really need a behavior match with Chrome on this?

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1253129 warn when using focused property with windows.create

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/082fb07b2a02
warn when using focused property with windows.create r=robwu

Comment 12

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_ext_windows_create_params.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/0b1a8f4cdacb55bc11dbf5ace9b031f6018c50d8
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304155015&repo=autoland&lineNumber=41039

Comment 13

[Shane Caraveo (:mixedpuppy)]

The test fails in debug with two leaking windows.

Passing "focused" as the test does, causes an error to be output in schema. Specifically, the error creation using cloneScope is what results in the leak (when sending the resulting object to Cu.reportError), I don't know why yet. Comment out the following lines[1], the leak no longer happens. Otherwise, changing the logError[2] function to use Cu.reportError(${error}); fixes it.

[1] https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/toolkit/components/extensions/Schemas.jsm#506-508
[2] https://searchfox.org/mozilla-central/rev/0e09b9191c02097034e46b193930f91c45b7885d/toolkit/components/extensions/Schemas.jsm#518

Comment 14

[Tomislav Jovanovic :zombie]

I imported the patch but don't see this, is the leak reproducing for you locally?

Anyway, is it a real leak, or just temporary (end of test)? Does it go away if you add a 10 second pause at the end of the test?

It could be possible we don't have many deprecated things on the child side, but I didn't look too deeply.

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to Tomislav Jovanovic :zombie from comment #14)

I imported the patch but don't see this, is the leak reproducing for you locally?

100% fail on osx debug build.

Anyway, is it a real leak, or just temporary (end of test)? Does it go away if you add a 10 second pause at the end of the test?

It's leaking, a delay like that didn't change anything.

Comment 16

[Rob Wu [:robwu]]

Does Cu.forceGC at the end of the test help?
cloneScope in an extension page is the window object of the background page, so when Cu.reportError is used on it, references to the error object are kept: https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/js/xpconnect/src/XPCComponents.cpp#1322-1457

but those references should be cleared when the background page is destroyed: https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/xpcom/base/nsConsoleService.cpp#523-530

Comment 17

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

I can reproduce the failure on both osx debug build and linux debug build.

Delaying exiting the test function or calling Cu.forceGC do not seem to help (and I also tried to explicitly clear all the message stored in the consoleService by manually calling nsIConsoleService.reset, but the detected shutdown leak was reproducible).

If this is a general issue, then I'm a bit surprised that this is the only test to trigger it, at a first glance the new test case added in the attached patch doesn't seem to be doing anything that we are not already doing.

I'm doing a full build locally and see if I can get some additional details about this.

Comment 18

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

I digged into it a bit more using rr and the Cu.reportError is not reporting the error with the innerWindowID of the context.cloneScope from which we are creating the Error instance, which means that consoleService will not be clearing out the scriptError instance when that extension global is being destroyed.

After digging more I've come to the conclusion that the shutdown leak detected is actually expected, because that error object would not be cleared from the consoleService and the docshell will be kept alive and freed when we are shutting down.

Given that the warning are not usually thrown to the caller but just logged using Cu.reportError, it sounds reasonable to not create the error as an instance of this.cloneScope.Error but pass it to Cu.reportError as a string and pass the expected caller stack using the second parameter, eg. like in the attached patch (which does prevent the shutdown leak when I tried it locally and still pass the test, and still provide the caller location).

In theory this change should still pass all other tests and still behave as the current implementation, but it would be good to push it to try to confirm that.

Pushed to try here (along with the other patch attached to this issue):

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=19f82a497896467bfb769cdd8cc212f1dc794143

Comment 19

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

Attachment: Bug 1253129 - Prevent a logged warning from keeping the extension global alive after being destroyed. r?mixedpuppy!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93f852ad5d28
warn when using focused property with windows.create r=robwu
https://hg.mozilla.org/integration/autoland/rev/1881ebd34b2b
Prevent a logged warning from keeping the extension global alive after being destroyed. r=mixedpuppy

Comment 21

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_ext_windows_create_params.js

backout: https://hg.mozilla.org/integration/autoland/rev/d0be929faa3de41faa2723860897bbfc572d0e2d

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1881ebd34b2b63e1fda3bff7620a5be34d8db76a&selectedTaskRun=dsSPpMFWRmmRg3ZS4Op2dg.1 but failed on tier-1 later on https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=JJiDUSekSBW1xlDKwQlLsw.0&revision=cafdc157b491c28f54af655e677e8a10dace3536&searchStr=linux%2C18.04%2Cx64%2Cshippable%2Copt%2Cmochitests%2Ctest-linux1804-64-shippable%2Fopt-mochitest-browser-chrome-e10s-6%2Cm%28bc6%29

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307910045&repo=autoland&lineNumber=1667

[task 2020-06-29T19:00:32.817Z] 19:00:32 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_windows_create_params.js | test result correct -
[task 2020-06-29T19:00:32.817Z] 19:00:32 INFO - Leaving test bound testWindowCreateParams
[task 2020-06-29T19:00:32.818Z] 19:00:32 INFO - Entering test bound testWindowCreateFocused
[task 2020-06-29T19:00:32.818Z] 19:00:32 INFO - Extension loaded
[task 2020-06-29T19:00:32.818Z] 19:00:32 INFO - Buffered messages finished
[task 2020-06-29T19:00:32.819Z] 19:00:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_windows_create_params.js | window is focused without focused param - Expected: false, Actual: true -
[task 2020-06-29T19:00:32.819Z] 19:00:32 INFO - Stack trace:
[task 2020-06-29T19:00:32.819Z] 19:00:32 INFO - chrome://mochikit/content/browser-test.js:test_ok:1299
[task 2020-06-29T19:00:32.819Z] 19:00:32 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:testHandler:68
[task 2020-06-29T19:00:32.820Z] 19:00:32 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:testResult:82
[task 2020-06-29T19:00:32.820Z] 19:00:32 INFO - resource://specialpowers/SpecialPowersChild.jsm:listener:2157
[task 2020-06-29T19:00:32.820Z] 19:00:32 INFO - resource://specialpowers/SpecialPowersChild.jsm:loadExtension/<:2099
[task 2020-06-29T19:00:32.821Z] 19:00:32 INFO - resource://specialpowers/SpecialPowersChild.jsm:receiveMessage:275
[task 2020-06-29T19:00:32.965Z] 19:00:32 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-06-29T19:00:32.969Z] 19:00:32 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_windows_create_params.js | window is focused with focused: true - Expected: false, Actual: true -
[task 2020-06-29T19:00:32.969Z] 19:00:32 INFO - Stack trace:
[task 2020-06-29T19:00:32.970Z] 19:00:32 INFO - chrome://mochikit/content/browser-test.js:test_ok:1299
[task 2020-06-29T19:00:32.970Z] 19:00:32 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:testHandler:68
[task 2020-06-29T19:00:32.970Z] 19:00:32 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:testResult:82
[task 2020-06-29T19:00:32.970Z] 19:00:32 INFO - resource://specialpowers/SpecialPowersChild.jsm:listener:2157
[task 2020-06-29T19:00:32.971Z] 19:00:32 INFO - resource://specialpowers/SpecialPowersChild.jsm:loadExtension/<:2099
[task 2020-06-29T19:00:32.971Z] 19:00:32 INFO - resource://specialpowers/SpecialPowersChild.jsm:receiveMessage:275
[task 2020-06-29T19:00:33.070Z] 19:00:33 INFO - Console message: [JavaScript Error: "Error: Warning processing focused: Opening inactive windows is not supported." {file: "moz-extension://7d26295b-c3d6-4050-9892-ebd99b195365/%7B0e0d0825-b5f1-463f-895c-cf38036d76a9%7D.js" line: 17}]
[task 2020-06-29T19:00:33.070Z] 19:00:33 INFO - background@moz-extension://7d26295b-c3d6-4050-9892-ebd99b195365/%7B0e0d0825-b5f1-463f-895c-cf38036d76a9%7D.js:17:40
[task 2020-06-29T19:00:33.070Z] 19:00:33 INFO -

Comment 22

[Shane Caraveo (:mixedpuppy)]

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/016006d107b6
warn when using focused property with windows.create r=robwu
https://hg.mozilla.org/integration/autoland/rev/ac8d0372dcb1
Prevent a logged warning from keeping the extension global alive after being destroyed. r=mixedpuppy

Comment 24

[Natalia Csoregi [:nataliaCs]]

Backed out for causing failures on browser_ext_windows_create_params.js

backout: https://hg.mozilla.org/integration/autoland/rev/6fa1a37eb1debf0aba26e1ec8508f48deb1692eb

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=ac8d0372dcb1ebcbcd188246e285078840a65be3

failure log: https://treeherder.mozilla.org/logviewer?job_id=324749498&repo=autoland&lineNumber=1967

[task 2020-12-16T21:48:40.585Z] 21:48:40 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_windows_create_params.js | test result correct -
[task 2020-12-16T21:48:40.585Z] 21:48:40 INFO - Leaving test bound testWindowCreateParams
[task 2020-12-16T21:48:40.586Z] 21:48:40 INFO - Entering test bound testWindowCreateFocused
[task 2020-12-16T21:48:40.586Z] 21:48:40 INFO - Extension loaded
[task 2020-12-16T21:48:40.587Z] 21:48:40 INFO - Buffered messages finished
[task 2020-12-16T21:48:40.587Z] 21:48:40 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_windows_create_params.js | Test timed out -
[task 2020-12-16T21:48:40.587Z] 21:48:40 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-12-16T21:48:40.587Z] 21:48:40 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_windows_create_params.js | Extension left running at test shutdown -
[task 2020-12-16T21:48:40.587Z] 21:48:40 INFO - Stack trace:
[task 2020-12-16T21:48:40.588Z] 21:48:40 INFO - chrome://mochikit/content/browser-test.js:test_ok:1304
[task 2020-12-16T21:48:40.588Z] 21:48:40 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:117
[task 2020-12-16T21:48:40.588Z] 21:48:40 INFO - chrome://mochikit/content/browser-test.js:nextTest:555
[task 2020-12-16T21:48:40.588Z] 21:48:40 INFO - GECKO(3984) | [Child 5212, Main Thread] WARNING: '!CanSend() || !mManager || !mManager->CanSend()', file /builds/worker/checkouts/gecko/dom/ipc/jsactor/JSWindowActorChild.cpp:40
[task 2020-12-16T21:48:40.588Z] 21:48:40 INFO - GECKO(3984) | [Child 5212: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0A53D400 == 3 [pid = 5212] [id = 5] [url = moz-extension://b4cadb96-0b99-4391-b80a-3cc09101f9e2/_generated_background_page.html]
[task 2020-12-16T21:48:40.588Z] 21:48:40 INFO - GECKO(3984) | MEMORY STAT | vsize 893MB | vsizeMaxContiguous 526MB | residentFast 274MB | heapAllocated 94MB
[task 2020-12-16T21:48:40.588Z] 21:48:40 INFO - TEST-OK | browser/components/extensions/test/browser/browser_ext_windows_create_params.js | took 90084ms

Comment 25

[Shane Caraveo (:mixedpuppy)]

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

Comment 26

[Shane Caraveo (:mixedpuppy)]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/406cfcadebd6
warn when using focused property with windows.create r=robwu
https://hg.mozilla.org/integration/autoland/rev/37a6b8654987
Prevent a logged warning from keeping the extension global alive after being destroyed. r=mixedpuppy

Comment 28

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/406cfcadebd6
https://hg.mozilla.org/mozilla-central/rev/37a6b8654987

Comment 29

[Denis Howe]

Wladimir Palant comment #2:

It would help if the windows.create() call wouldn't error when seeing focused=true.

v92 (mac) says "Error: Warning processing focused: Opening inactive windows is not supported." when I pass focused: false.

Will Bamberg [:wbamberg] comment #3:

I've updated the docs page.

So should
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/windows/create
mention the fact that this property is deprecated?

Comment 30

[Richard Bloor]

BCD includes the note "From Firefox 86, the focused: false option is ignored." and there is a note in the release notes.