799592 - Add nsIDOMWindowUtils API to allow marking windows as closable-from-script regardless of dom.allow_scripts_to_close_windows pref value

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Shane Caraveo (:mixedpuppy)]

bug 795518 introduced a DOMWindowClose listener so we could allow social panel content to close itself.  However, it only works if dom.allow_scripts_to_close_windows is true, the default is false.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: use closePanel api.patch

Changing https://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6562 seems like too much a hassle to get this into 17 (if we can), using an app frame doesn't look like it would work, so I think we're kind of left with using a mozSocial api.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: use closePanel api.patch

also fixes tests

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: stick with window.close but bypass the preference

The following patch reproduces the original problem in the test code, and also arranges for window.close() to be called via chrome code, meaning that preference isn't used.  The patch seems sane to me, but I'm not sure this is the correct way to replace window.close - jaws, do you happen to know if that is kosher?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 669708 [details] [diff] [review]
use closePanel api.patch

Review of attachment 669708 [details] [diff] [review]:
-----------------------------------------------------------------

The closePanel for chatboxes is kinda hacky, but it seems like the easiest route for this late in the release cycle.

::: toolkit/components/social/MozSocialAPI.jsm
@@ +135,5 @@
>        configurable: true,
>        writable: true,
>        value: function(toURL, offset, callback) {
> +        // if called and this is in any popup panel, close the panel
> +        // does not close sidebar

This comment doesn't reference chatboxes.

::: toolkit/components/social/WorkerAPI.jsm
@@ +64,5 @@
>        let results = [];
>        cookies.forEach(function(aCookie) {
>          let [name, value] = aCookie.split("=");
>          results.push({name: unescape(name.trim()),
> +                      value: value && unescape(value.trim())});

value: value ? unescape(value.trim()) : ""});

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 669850 [details] [diff] [review]
stick with window.close but bypass the preference

Review of attachment 669850 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure which patch here is the one to take. This one obviously seems simpler and providers a nicer API.

::: browser/base/content/test/browser_social_flyout.js
@@ +98,4 @@
>            iframe.contentDocument.addEventListener("SocialTest-DoneCloseSelf", function _doneHandler() {
>              iframe.contentDocument.removeEventListener("SocialTest-DoneCloseSelf", _doneHandler, false);
>              is(panel.state, "closed", "flyout should have closed itself");
> +            Services.prefs.setBoolPref(ALLOW_SCRIPTS_TO_CLOSE_PREF, oldAllowScriptsToClose);

The oldAllowScriptsToClose variable here is unnecessary. Instead, we can just call Services.prefs.clearUserPref(ALLOW_SCRIPTS_TO_CLOSE_PREF);

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Felipe, which route do you think we should take?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

If that's all it takes to have window.close() working for the panels I say we should take it.
Can you delete the expando on unload?


Olli: it'd be nice if you took a quick look at this bug and see if this patch is fine (attachment 669850 [details] [diff] [review]).

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 669850 [details] [diff] [review]
stick with window.close but bypass the preference

IMO window.close is preferable over a new api

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 669850 [details] [diff] [review]
stick with window.close but bypass the preference

Looks like a hack to me, and something which won't do the right thing
once window object is WebIDLfied, I think. (methods should be in the prototype.)
I'd add a boolean member variable mAllowScriptsToClose to nsGlobalWindow and then
using DOMWindowUtils one could set that variable.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

If we don't mind touching nsGlobalWindow, another option would be to perform the preference check later, after the DOMWindowClose event - we always cancel the close during that event anyway (we just hide the panel).

How do you feel about getting this version landed (a social-only change we can possibly uplift) and having another bug to remove it once nsGlobalWindow can play along?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I think we can possibly uplift the nsGlobalWindow change you describe, since it sounds simple enough. Can we get a trunk patch for that and then evaluate risk, before we go for the hacky approach?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: compiles, untested

So I was thinking something like this.
Anyone want to test?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Created attachment 669962 [details] [diff] [review]
> compiles, untested
> 
> So I was thinking something like this.
> Anyone want to test?

I'm happy to test it (am about to build), but would this be able to uplift to 17?  Marks patch is isolated to just the socialapi code.  I think we should use Marks patch for 17, maybe 18, and this for 19.

When does the WebIDL stuff happen?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

not soon.

But actually, even now close is in the prototype.
window.hasOwnProperty("close"); is false,
window.__proto__.hasOwnProperty("close"); is true.

As far as I see, the JS patch would make window.hasOwnProperty("close"); true.

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Created attachment 669962 [details] [diff] [review]
> compiles, untested
> 
> So I was thinking something like this.
> Anyone want to test?

This patch, in combination with Marks original patch that listened for DOMWindowClose, works fine for using window.close.  If however, the panel doesn't actually get closed (I first forgot to reapply Marks patch), I am able to call window.close multiple times, resulting in a console error:

WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x8000FFFF: file /Users/shanec/moz/mozilla-central/content/base/src/nsContentUtils.cpp, line 3014

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

What you mean "doesn't actually get closed?"
DOMWindowClose event should have been cancelled if panel isn't about to be closed.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Olli Pettay [:smaug] from comment #16)
> What you mean "doesn't actually get closed?"
> DOMWindowClose event should have been cancelled if panel isn't about to be
> closed.

We're doing this for content loaded in a frame in a xul:panel.  to my knowledge, panels are not closed by window.close.  Marks patch added the ability to use window.close in that context by closing the panel when receiving DOMWindowClose events.

When I applied your patch, I forgot to also apply Marks patch.  I had a DOMWindowClose listener in my test content as well that just did a dump, which I did see.  So the event fired, but without Marks patch the panel didn't close.  In this situation, subsequent attempts to call window.close after the first call did NOT fire DOMWindowClose, but gave the console error instead.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

so the patch works fine if you handle DOMWindowClose the way it should be handled, right?

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Olli Pettay [:smaug] from comment #18)
> so the patch works fine if you handle DOMWindowClose the way it should be
> handled, right?

Yes.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Attachment: check can-close preference after DOMWindowClose event

Move the preference check to after the event (ie, this patch just moves a block and adds a comment)

This is what I had in mind - it should have the same effect but doesn't involve new member variables or changes to any interfaces (so no uuid bump etc).  It is probably riskier for uplift, but seems better longer term.

It also avoids the slightly ironic situation of adding a new variable allowing scripts to close windows when our use case *doesn't* actually close the window!

This is just for reference though - the most important thing is probably to agree on a patch we can uplift...

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Why is that better? The meaning of DOMWindowClose changes quite a bit.
Right now it means "The window is about to be closed".
With your patch "The window may be closed, if certain preference isn't set and the"

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Fair enough - it certainly is subjective :)

So you are proposing your patch for 17?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 669962 [details] [diff] [review]
compiles, untested

This should be supersafe, so I think FF17 should be ok.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Attachment: beta patch

Trivial merge differences, different UUID for nsIDOMWindowUtils.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 669962 [details] [diff] [review]
compiles, untested

This patch applies as-is to Aurora.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
* this implements a new API, doesn't fix a bug
User impact if declined:
* new Social API functionality requires this for social panels to behave correctly
Testing completed (on m-c, etc.):
* on m-c since Friday, tested by Social API tests
Risk to taking this patch (and alternatives if risky):
* low risk addition to nsIDOMWindowUtils API, purely additive, doesn't affect content or anyone not using it in any way
String or UUID changes made by this patch:
* UUID change to nsIDOMWindowUtils, shouldn't present any compatibility issues

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 671726 [details] [diff] [review]
beta patch

[Approval Request Comment]
(see comment 26)

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 671726 [details] [diff] [review]
beta patch

not a bug fix, new feature implementation targeted for 17, approving.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/05afca9f8a70
https://hg.mozilla.org/releases/mozilla-beta/rev/8600f7c9c1c0