845893 - window.close() on a private window doesn't pass through warnAboutClosingWindow

Status: RESOLVED WONTFIX

(Firefox :: Private Browsing, defect)

Description

[Josh Matthews [:jdm]]

Therefore we don't get any last-pb-context-exiting warning.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Make window.close() on a private chrome window trigger last-pb-context-exiting.

Comment 2

[(no longer active)]

At some point we should probably stop having more than 285 ways of closing windows. :(

Comment 3

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

Comment on attachment 719103 [details] [diff] [review]
Make window.close() on a private chrome window trigger last-pb-context-exiting.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+window.addEventListener('DOMWindowClose', function windowShouldClose(ev) {
>+  // Usually window.close() on a chrome window is not blockable. If it's a
>+  // private window, however, we want to prevent potential dataloss.
>+  if (PrivateBrowsingUtils.isWindowPrivate(window) &&
>+      !warnAboutClosingWindow()) {
>+    ev.preventDefault();
>+  }
>+  window.removeEventListener('DOMWindowClose', windowShouldClose, false);

You don't actually want to remove this listener, do you? Otherwise it looks to me like the second window.close() call for a given window will be broken.

Comment 4

[Josh Matthews [:jdm]]

Oops, in my head there was an early return in the if block.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Ehsan Akhgari from comment #2)
> At some point we should probably stop having more than 285 ways of closing
> windows. :(

Right, and it seems like this bug wants to add another one.

As far as I remember, window.close isn't handled (e.g. no warnings for the user, no browser-lastwindow-close-* notifications) because UI code shouldn't be calling it in the first place. So, what code is calling window.close?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Ehsan Akhgari from comment #2)
> > At some point we should probably stop having more than 285 ways of closing
> > windows. :(
> 
> Right, and it seems like this bug wants to add another one.

Yeah, that was my first thought, too. It's bad the we can get into strange states when some code calls window.close() but I think this should maybe be handled by PrivateBrowsingUtils? Can't we make that keep track of how many private windows there are and send a notification if necessary?

I know that this is more like some tools instead of a service that wants to keep track but OTOH these last-pb-context-exiting/exited notification seem actually weird to me now that we have per-window private browsing.

Comment 7

[Josh Matthews [:jdm]]

If everyone agrees that we don't deal with window.close, then we can just WONTFIX this. I was calling it in a test and lost an hour to this problem.

Comment 8

[(no longer active)]

(In reply to comment #5)
> (In reply to :Ehsan Akhgari from comment #2)
> > At some point we should probably stop having more than 285 ways of closing
> > windows. :(
> 
> Right, and it seems like this bug wants to add another one.
> 
> As far as I remember, window.close isn't handled (e.g. no warnings for the
> user, no browser-lastwindow-close-* notifications) because UI code shouldn't be
> calling it in the first place. So, what code is calling window.close?

Erm I thought this is being triggered by a web page calling window.close(), is that not the case?

Comment 9

[(no longer active)]

(In reply to comment #7)
> If everyone agrees that we don't deal with window.close, then we can just
> WONTFIX this. I was calling it in a test and lost an hour to this problem.

No, hold on.  Unless I'm missing something, we should absolutely fix this bug, as content can close the windows it has opened.

Comment 10

[Josh Matthews [:jdm]]

I believe this is only a problem when calling close on a chrome window object.

Comment 11

[(no longer active)]

(In reply to comment #10)
> I believe this is only a problem when calling close on a chrome window object.

Ah ok.  In that case... maybe we can just wontfix this?  :/

Comment 12

[(no longer active)]

Please reopen if you disagree.