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

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
7 years ago
6 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Therefore we don't get any last-pb-context-exiting warning.
Assignee: nobody → josh
At some point we should probably stop having more than 285 ways of closing windows. :(
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.
Oops, in my head there was an early return in the if block.
(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?
(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.
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.
(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?
(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.
I believe this is only a problem when calling close on a chrome window object.
(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?  :/
Attachment #719103 - Flags: review?(ttaubert)
Please reopen if you disagree.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.