Closed Bug 1253129 Opened 8 years ago Closed 3 years ago

Support focused=false in the browser.windows.create

Categories

(WebExtensions :: Frontend, defect, P3)

defect

Tracking

(firefox86 fixed)

RESOLVED FIXED
86 Branch
Iteration:
48.1 - Mar 21
Tracking Status
firefox86 --- fixed

People

(Reporter: kmag, Assigned: mixedpuppy)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [windows]triaged)

Attachments

(2 files)

Chrome creates these windows with the normal stacking order and positioning, but does not focus them.
Assignee: nobody → kmaglione+bmo
Iteration: --- → 48.1 - Mar 21
Priority: -- → P3
Target Milestone: --- → mozilla49
Whiteboard: [windows] → [windows]triaged
Target Milestone: mozilla49 → ---
Component: WebExtensions: Untriaged → WebExtensions: Frontend
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.
(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.
(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.
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
Product: Toolkit → WebExtensions
Assignee: kmaglione+bmo → nobody

(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

See Also: → 1235231

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.

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.

Flags: needinfo?(mdeboer)

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?

Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/082fb07b2a02
warn when using focused property with windows.create r=robwu

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

Flags: needinfo?(mixedpuppy) → needinfo?(tomica)

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.

Flags: needinfo?(tomica)

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

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

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.

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

Pushed by scaraveo@mozilla.com:
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

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 -

Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy)
Pushed by scaraveo@mozilla.com:
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

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

Flags: needinfo?(mixedpuppy)
Pushed by scaraveo@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
Flags: needinfo?(mixedpuppy)

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?

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

See Also: → 1800360
Duplicate of this bug: 1800360
Duplicate of this bug: 1806020
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: