Closed Bug 1301227 Opened 8 years ago Closed 8 years ago

Allow windows created from windows.create() to close themselves

Categories

(WebExtensions :: Frontend, defect, P3)

Product:
WebExtensions
Component:
Frontend
Version:
51 Branch
Type:
defect

Tracking

(firefox51 wontfix, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: aswan, Assigned: zombie)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved]triaged)

Attachments

(1 file)

bug 1301227 - add allowScriptsToClose option to windows.create(),
58 bytes, text/x-review-board-request
kmag
: review+
Details 
When a window created with browser.windows.create() tries to close itself with the dom window.close() API, the window isn't closed and the following error is logged:
Scripts may not close windows that were not opened by script.

These windows are effectively opened by a script, we should teach the code that tracks this about windows created in this way so that they can be closed from a script.
I'm on the fence about this. Chrome does seem to allow it, and you can make an argument that it makes sense, but I'm not sure it actually does.

The allow scripts to close behavior is meant to allow windows opened by content pages to close themselves. Windows opened by extensions aren't really in the same class, though. A window opened from a bookmark, by a bookmark extension, because of some user interaction, for instance, should probably behave like a window opened by a user, from the bookmarks menu, rather than one opened by `window.open()` called from a content page.

Are there any particular use cases that require this?
Whiteboard: [design-decision-needed]
(In reply to Kris Maglione [:kmag] from comment #1)
> Are there any particular use cases that require this?

I came across it trying to emulate a dialog box.  The extension does send a message back to the background page with the results when it closes, so it wouldn't be unreasonable for the background page to close the window itself, but in my first cut, I wasn't keeping track of the created window since I assumed it could just close itself.

How about a property in the new window options object to let the caller explicitly allow it?  (defaulting to false for the reasons you cited above would be fine)
(In reply to Andrew Swan [:aswan] from comment #2)
> How about a property in the new window options object to let the caller
> explicitly allow it?  (defaulting to false for the reasons you cited above
> would be fine)

I'd be OK with that. It might make sense to default to true when opening windows/tabs with extension URLs, though.
Priority: -- → P3
Whiteboard: [design-decision-needed] → [design-decision-needed]triaged
For whoever picks this bug up, it looks like this should be simple.  In the scenarios where we want to allow the window to close itself, we just need to get a handle to the window's nsIDOMWindowUtils interface and call this method:
http://searchfox.org/mozilla-central/rev/124c120ce9d7e8407c9ad330a9d6b1c4ad432637/dom/interfaces/base/nsIDOMWindowUtils.idl#1761
Comment on attachment 8794404 [details]
bug 1301227 - add allowScriptsToClose option to windows.create(),

I tried to make a test for this option (attached mozreview), but it passes even without any changes to firefox code. It looks like some privilege/context is preserved when running within the mochitest framework, so the code is always allowed to close the window.

I tried a number of ways to hack around this: 
 - opening a page from the example.com http server (as opposed to extension files),
 - using setTimeout (trying to break the "window is opened from a script" chain),
 - even using AddonManager.jsm to install an external temporary extension.
and nothing made any difference.

Kris, do you have any idea how to work around this?
Attachment #8794404 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8794404 [details]
bug 1301227 - add allowScriptsToClose option to windows.create(),

(Kris Maglione [:kmag] from irc)
> Apparently we set dom.allow_scripts_to_close_windows to true for tests,
> so you just need to use SimpleTest.pushPrefEnv to set it to false.
Attachment #8794404 - Flags: feedback?(kmaglione+bmo)
Assignee: nobody → tomica
Status: NEW → ASSIGNED
After coding and going through all the possible test cases for this, I now think we might not wont to do this. Basically, there is a fundamental mismatch between windows and tabs for this api. DOMWindowUtils operates on a single DOMWindow (tab), while `browser.windows` api is on the level of browser windows.

Next, there are many ways of doing "open this url in a new (browser) window": passing a url to windows.create(); opening a tab in the current window and passing tabid to window.create(); opening a new window, then loading the url into the activeTab, etc... To make this option behave consistently, we would have to set an internal per-window flag, and track all those, and other possible ways of opening/navigating to a tab inside the window, and applying the flag to each one.

And even after all that, it would still *only work when there is just one tab in the window*. The moment either the extension or the user (or even the page itself) opens a second tab, it would fail to close the window (and possibly introduce unexpected bugs to extensions that depend on it).

IMHO, it would be too much work and too messy api (with unexpected outcomes), for something that is basically a "convenience feature" to avoid passing a single message to the background page (where windows and tabs are properly distinguished in the api).
In the case of `windows.create`, the flag should only be applied to the individual tab or tabs opened by the call (the same as would happen via `window.open()`)
Whiteboard: [design-decision-needed]triaged → [design-decision-approved]triaged
Comment on attachment 8794404 [details]
bug 1301227 - add allowScriptsToClose option to windows.create(),

https://reviewboard.mozilla.org/r/80890/#review82658

::: browser/base/content/tab-content.js:933
(Diff revision 6)
>  addEventListener("unload", () => {
>    ExtensionContent.uninit(this);
>    RefreshBlocker.uninit();
>  });
>  
> +addMessageListener("Extension:AllowScriptsToClose", () => {

No `Extension:` prefix except for extension-specific messages, please.

::: browser/base/content/tab-content.js:934
(Diff revision 6)
>    ExtensionContent.uninit(this);
>    RefreshBlocker.uninit();
>  });
>  
> +addMessageListener("Extension:AllowScriptsToClose", () => {
> +  content.getInterface(Ci.nsIDOMWindowUtils).allowScriptsToClose();

`content.QueryInterface(Ci.nsIInterfaceRequestor)...`

I know it works most of the time without the `QueryInterface`, but it relies on someone else doing the QI before we do, so is liable to break.

::: browser/components/extensions/ext-windows.js:174
(Diff revision 6)
> -
>              resolve();
>            });
>          }).then(() => {
> +          if (allowScriptsToClose) {
> +            for (let {linkedBrowser} of window.gBrowser.tabs) {

We should probably do this after delayed-startup-finished if we're passed multiple URLs... but we can file a follow-up bug for that.

Oh, and should probably forbid `allowScriptsToClose` in conjunction with `tabId`...

::: browser/components/extensions/test/browser/browser_ext_windows_create.js:206
(Diff revision 6)
> +  yield SpecialPowers.pushPrefEnv({set: [["dom.allow_scripts_to_close_windows", false]]});
> +
> +  yield extension.startup();
> +  yield extension.awaitFinish();
> +
> +  let win = BrowserTestUtils.waitForNewWindow();

This should probably be called something like `windowPromise`.

But it might be better to just do `let win = yield BrowserTestUtils.waitForNewWindow()` after we send the message. It shouldn't be possible for that message to be handled and a window to be open until we return to the event loop again, anyway.
Attachment #8794404 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8794404 [details]
bug 1301227 - add allowScriptsToClose option to windows.create(),

https://reviewboard.mozilla.org/r/80890/#review82658

> We should probably do this after delayed-startup-finished if we're passed multiple URLs... but we can file a follow-up bug for that.
> 
> Oh, and should probably forbid `allowScriptsToClose` in conjunction with `tabId`...

well, multiple URLs wouldn't work anyway (wont't close a window with multiple tabs), and delayed-browser-finished already needs to be handled for bug 1273146 so I don't think we need a bug for that.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5322af204cb0
add allowScriptsToClose option to windows.create(), r=kmag
Keywords: checkin-needed
Comment on attachment 8794404 [details]
bug 1301227 - add allowScriptsToClose option to windows.create(),

https://reviewboard.mozilla.org/r/80890/#review82658

> well, multiple URLs wouldn't work anyway (wont't close a window with multiple tabs), and delayed-browser-finished already needs to be handled for bug 1273146 so I don't think we need a bug for that.

The individual tabs should still be able to close themselves, even if closing only one of them won't close the browser window.
https://hg.mozilla.org/mozilla-central/rev/5322af204cb0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1402695
Keywords: dev-doc-needed
(In reply to Andrew Swan [:aswan] from comment #2)
> (In reply to Kris Maglione [:kmag] from comment #1)
> > Are there any particular use cases that require this?
> 
> I came across it trying to emulate a dialog box.  The extension does send a
> message back to the background page with the results when it closes, so it
> wouldn't be unreasonable for the background page to close the window itself,
> but in my first cut, I wasn't keeping track of the created window since I
> assumed it could just close itself.

chrome.windows.remove can be used here.
Or, since popup windows have only one tab,
chrome.tabs.remove can also be used.

In bug 1402695 I showed that nobody is using this new property.
The source code and bug are public, so if the feature served any real need, then I would expect at least one use in the wild by now. This is not the case, so I want to get rid of the allowScriptsToClose parameter to simplify the API.


The only potential advantage of window.close over windows.remove is that window.close can be called from a page or content script.
That is a very rare use case and does not require an extra parameter to browser.windows.create, as it can be implemented with a content script like this:

// content script:
exportFunction(function() {
  browser.runtime.sendMessage('closeme');
}, window, {defineAs: 'close'});

// background script
browser.runtime.onMessage.addListener((msg, sender) => {
  if (msg === 'closeme') {
    browser.windows.remove(sender.tab.windowId);
  }
});


Andrew, do you see any need to keep this allowScriptsToClose property around, or can it be removed (re-open bug 1402695)?
Flags: needinfo?(aswan)
I think I was one of the original requestors for this capability.  My reasoning was that we want to enable extension authors to use vanilla DOM APIs as much as possible and it was surprising that window.close() didn't work from an extension's own window.  The code from comment 21 would work of course, but I prefer just making the existing API that experienced web developers will already know work, and in this case I don't think it adds unnecessary complexity or overhead...
Flags: needinfo?(aswan)
(In reply to Andrew Swan [:aswan] from comment #22)
> ..., but I prefer just making the existing API that experienced web
> developers will already know work

window.close() does not work on the web, except when explicitly created by a script, via window.open().
Since the window is opened via the extension API, it is completely reasonable to require extension APIs to close it.


> and in this case I don't think it adds unnecessary complexity or overhead...

If the API becomes public we have to commit to maintaining the API, even if things change internally. The API is not implemented anywhere else either, and due to its limited use has no chance of ever becoming part of a standard.

There is no actual benefit to offset this non-zero cost, so I believe that it is better for the future to get rid of this unused API now.

(also removing dev-doc-needed until this discussion has finished, because removal will be more difficult once documented).
Keywords: dev-doc-needed
(In reply to Rob Wu [:robwu] from comment #23)
> > and in this case I don't think it adds unnecessary complexity or overhead...
> 
> If the API becomes public we have to commit to maintaining the API, even if
> things change internally. The API is not implemented anywhere else either,
> and due to its limited use has no chance of ever becoming part of a standard.

I'm not sure what sort of internal change you're anticipating that would make this difficult to support.
I also don't agree that it "has no chance of ever becoming part of a standard".  That's not to say that I or anybody else is likely to try to standardize it soon but "no chance" is an absolute statement.

> There is no actual benefit to offset this non-zero cost, so I believe that
> it is better for the future to get rid of this unused API now.

"no actual benefit" is again an absolute statement about a subjective thing.  I'd be fine sending this to the webextensions api triage meeting if you prefer that...
Here are some things to consider:

1) Someone spent a lot of time and effort, very recently, to implement this feature. Filing a peremptory bug to remove it, without event discussing it first, seriously undervalues his contribution, and I don't appreciate it. Please don't assume that we're idiots or that you automatically know more than us. If you disagree with a decision we've made, you know where to find us.

2) Tomica is a peer of this module. His decisions have exactly the same weight as Andrew's. If you disagree with them, then the person you need to discuss it with is Tomica. Please don't publicly ask someone else to overrule him. He's earned those privileges and responsibilities, and I won't stand for them being disrespected.

3) The behavior of window.close() is not the same as browser.windows.remove(). The former takes into account things like other tabs in the same browser window, and adjusts its behavior as appropriate. Asking each extension to try to emulate that behavior manually is only asking for trouble.

4) The definition of "script-created windows" is not exactly clear as it applies to APIs like browser.windows.create(). Intuitively, many people expected it to apply to windows created using that API, which is why this bug was filed to begin with.

5) We already support window.close() in other extension contexts, in particular, browserAction and pageAction popups. Those *are not* script created, but we support them just the same.

I want to be very clear: This is the end of this discussion. If you want to discuss this further, take it to IRC or the mailing list. But only if you can do so respectfully.
Keywords: dev-doc-needed
Apologies if I came off ass disrespectful or pushy. That was not intentional. I value all of your contributions, and time especially, so in my attempt to write as concise as possible, I might have given the wrong vibe.


(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #25)
> 1) Someone spent a lot of time and effort, very recently, to implement this
> feature. Filing a peremptory bug to remove it,

The patch does two things:
- Allowing moz-extension:-pages to unconditionally call window.close().
- Allowing other page types to call window.close() if allowScriptsToClose is passed to windows.create.

I proposed to remove only the latter, and KEEP the first.
So most of the code is kept, except for the part that accepts the allowScriptsToClose option in browser.windows.create.

You probably know already, but for completeness.
- The allowScriptsToClose option does NOT allow new tabs in the window to call window.close()
- Contrary to Chrome (see below), pages to which the allowScriptsToClose option is applied (including moz-extension:) will keep the ability to close the tab via window.close() after navigation.



> 2) Tomica is a peer of this module. His decisions have exactly the same
> weight as Andrew's. If you disagree with them, then the person you need to
> discuss it with is Tomica.

In https://bugzilla.mozilla.org/show_bug.cgi?id=1402695#c1, he said "considered useful enough to add by several people/design team." Since he refers to others, I thought that it was appropriate to ask the one who created the feature request (Andrew) for concrete use cases.

In comment 2, Andrew mentioned the use case of a dialog box. Even after removing the allowScriptsToClose option, window.close() can still be used to close extension dialog boxes.

In comment 1, you (Kris) say "Chrome does seem to allow it". That seems so, but it often is not.
Chromium only permits tabs to close themselves via window.close() if the page is the first item in the history (even calling history.pushState will already break window.close). Popups created by window.open and chrome.runtime.create have only one history item, so it may seem that Chrome does some special handling for popups, but that is not the case.

If you open example.com for example, then window.close() will close the window.
However, if you click on the link in example.com, then window.close() will silently fail, and Chrome logs the following message: "Scripts may close only the windows that were opened by it."



 
> 3) The behavior of window.close() is not the same as
> browser.windows.remove(). The former takes into account things like other
> tabs in the same browser window, and adjusts its behavior as appropriate.

window.close() is closer to chrome.tabs.remove.
But popups can only have one tab (both in Firefox and Chrome), so it is also equivalent to chrome.windows.remove when {type:'popup'} was set.

The option does not allow new tabs in the window to close themselves, hence I ignored the situation of multiple tabs.



> 4) The definition of "script-created windows" is not exactly clear as it
> applies to APIs like browser.windows.create(). Intuitively, many people
> expected it to apply to windows created using that API, which is why this
> bug was filed to begin with.

Did people (extension developers?) expect it to apply to web pages too?
Comment 2's use case is a dialog box (which is incidentally also why I looked into this API), and that continues to be possible even without the allowScriptsToClose option.


> 5) We already support window.close() in other extension contexts, in
> particular, browserAction and pageAction popups. Those *are not* script
> created, but we support them just the same.

moz-extension:-pages will still keep the ability to call window.close(), even after removing the new option.


> This is the end of this discussion.

(not expecting you to reply here, I just want to record my response in this bug because I believe that I add new information.)




(In reply to Andrew Swan [:aswan] from comment #24)
> I also don't agree that it "has no chance of ever becoming part of a
> standard".  That's not to say that I or anybody else is likely to try to
> standardize it soon but "no chance" is an absolute statement.
> 
> "no actual benefit" is again an absolute statement about a subjective thing.

For an API to become standardized, more than one vendor must be willing to implement it.
I can't speak for anyone but myself, but unless a compelling use case is given, I consider the likelihood of supporting an allowScriptsToClose parameter close to zero.
For the record, I didn't find Rob's discussion disrespectful, except maybe for undoing of my dev-doc-needed flag.


As for the discussion on the bug itself, we are not aiming for 100% compatibility with Chrome, the Browser Extensions standard or any other external entity.  Something being "unlikely to ever be a part of standard" is not enough of a reason not to implement it if we find it useful, or we will end up with a "lowest common denominator" set of features.

Compatibility is a valid concern for a feature if it hinders developers in writing cross-browser code, but this one doesn't fit that description.
ff 57.0b5 on macos 10.12: works as advertised

ff 57.0b5 on windows 10:

chrome.windows.create({ url: 'pages/login-panel.html' }, (win) => {
  console.error(win.id);
});

 - window is opened and page displayed
 - window.close fails with 'Scripts may not close...'

regression?
Not sure how it works on OSX, but in general, the url needs to start with "moz-extension://" (so use browser.runtime.getURL), or you need to pass the `allowScriptsToClose: true` option.
I've had a go at documenting this here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/create

Please let me know if this covers it.
Flags: needinfo?(tomica)
awesome. thanks, will.
awesome. thanks Will.
Flags: needinfo?(tomica)
See Also: → 1405265
Depends on: 1418394
I am having an issue with window.close() error on windows.create()
While it is not a major issue and I can close the window with chrome.windows.remove() I got confused by the MDN on https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/windows/create

Firefox 59.0a1 (2017-11-15) (64-bit)
Example code:


Background script
--------------------
chrome.windows.create({
  url: 'content/popup.html', 
  type: 'panel',
  width: 140,
  height: 80    
}, win => {});


popup.html
--------------------
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>Popup</title>
  </head>
  <body>
    <article>
      <span>...</span>
    </article>
  <script src="popup.js"></script>
  </body>
</html>


popup.js
--------------------
'use strict';
window.close();


It results in "Scripts may not close windows that were not opened by script."
N.B. The resulting URL is: moz-extension://*****/content/popup.html


I have tried the following but result remain the same:
allowScriptsToClose: true
chrome.runtime.getURL('content/popup.html')


Has the behaviour changed or is this a bug, or am I missing something?


PS. Why both type: 'panel', & type: 'popup', produces the same popup type?

Object { id: 361, focused: true, top: 334, left: 583, width: 200, height: 70, 
        incognito: false, type: "popup", state: "normal", alwaysOnTop: false, 
        title:"Nightly", tabs:Array[1]}
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: