The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: smaug)

Tracking

unspecified
mozilla19
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed)

Details

(Whiteboard: [Fx17][qa-])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 669703 [details] [diff] [review]
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.
Attachment #669703 - Flags: review?(jaws)
Attachment #669703 - Flags: feedback?(mhammond)
(Reporter)

Comment 2

5 years ago
Created attachment 669708 [details] [diff] [review]
use closePanel api.patch

also fixes tests
Attachment #669703 - Attachment is obsolete: true
Attachment #669703 - Flags: review?(jaws)
Attachment #669703 - Flags: feedback?(mhammond)
Attachment #669708 - Flags: review?(jaws)
Attachment #669708 - Flags: feedback?(mhammond)
(Reporter)

Updated

5 years ago
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
(Reporter)

Updated

5 years ago
Whiteboard: [Fx17]
Created attachment 669850 [details] [diff] [review]
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?
Attachment #669850 - Flags: feedback?(mixedpuppy)
Attachment #669850 - Flags: feedback?(jaws)
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()) : ""});
Attachment #669708 - Flags: review?(jaws) → review+
Attachment #669850 - Attachment is patch: true
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);
Attachment #669850 - Flags: feedback?(jaws) → feedback+
Felipe, which route do you think we should take?
Flags: needinfo?(felipc)
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]).
Flags: needinfo?(felipc)
(Reporter)

Comment 8

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

IMO window.close is preferable over a new api
Attachment #669850 - Flags: feedback?(mixedpuppy) → feedback+
(Assignee)

Comment 9

5 years ago
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.
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?
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?
(Assignee)

Comment 12

5 years ago
Created attachment 669962 [details] [diff] [review]
compiles, untested

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

Comment 13

5 years ago
(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?
(Assignee)

Comment 14

5 years ago
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.
(Reporter)

Comment 15

5 years ago
(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
(Assignee)

Comment 16

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

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
so the patch works fine if you handle DOMWindowClose the way it should be handled, right?
(Reporter)

Comment 19

5 years ago
(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.

Updated

5 years ago
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Created attachment 670211 [details] [diff] [review]
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...
(Assignee)

Comment 21

5 years ago
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"
Fair enough - it certainly is subjective :)

So you are proposing your patch for 17?
(Assignee)

Comment 23

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

This should be supersafe, so I think FF17 should be ok.
Attachment #669962 - Flags: review?(jst)

Updated

5 years ago
Attachment #669962 - Flags: review?(jst) → review+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b1775e08bb16
Blocks: 801080
Component: SocialAPI → DOM
Product: Firefox → Core
Summary: window.close will not close social panels → Add API to allow marking windows as closable-from-script regardless of dom.allow_scripts_to_close_windows pref value
Assignee: nobody → bugs
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Summary: Add API to allow marking windows as closable-from-script regardless of dom.allow_scripts_to_close_windows pref value → Add nsIDOMWindowUtils API to allow marking windows as closable-from-script regardless of dom.allow_scripts_to_close_windows pref value
Target Milestone: --- → mozilla19
Attachment #670211 - Attachment is obsolete: true
Attachment #669850 - Attachment is obsolete: true
Attachment #669708 - Attachment is obsolete: true
Attachment #669708 - Flags: feedback?(mhammond)
Created attachment 671726 [details] [diff] [review]
beta patch

Trivial merge differences, different UUID for nsIDOMWindowUtils.
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
Attachment #669962 - Flags: approval-mozilla-aurora?
Comment on attachment 671726 [details] [diff] [review]
beta patch

[Approval Request Comment]
(see comment 26)
Attachment #671726 - Flags: approval-mozilla-beta?
Comment on attachment 671726 [details] [diff] [review]
beta patch

not a bug fix, new feature implementation targeted for 17, approving.
Attachment #671726 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #669962 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/05afca9f8a70
https://hg.mozilla.org/releases/mozilla-beta/rev/8600f7c9c1c0
status-firefox17: --- → fixed
status-firefox18: --- → fixed
Whiteboard: [Fx17] → [Fx17][qa-]
You need to log in before you can comment on or make changes to this bug.