462639 - Handle view-source windows in Private Browsing mode

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P3)

Description

[(no longer active)]

We currently ignore view source windows during transitions to and from private browsing mode.  I think we need to handle them in the following manner:

Upon entry: record a list of open view source windows, and close them together with current browser windows.

Upon leaving: close all of the open view source windows, and re-open the recorded list after restoring the session.

Simon, we currently do not handle view source windows in the session store component, right?

Comment 1

[(no longer active)]

Another thing we need to handle is to add "(Private Browsing)" to the title of view-source windows while inside the private browsing mode.

Comment 2

[Simon Bünzli]

(In reply to comment #0)
> we currently do not handle view source windows in the session store
> component, right?

Indeed, we don't handle anything but browser windows (see bug 385084).

Comment 3

[(no longer active)]

(In reply to comment #2)
> Indeed, we don't handle anything but browser windows (see bug 385084).

Thanks!  So I guess I'd have to do the dirty work myself.  Of course I think only opening the windows should be enough, and we shouldn't have to restore scroll positions, etc.

Comment 4

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 5

[Nochum Sossonko [:Natch]]

Open View Source windows in PB are not closed on exit in the current impl. nom'ing for blocking, as this directly defeats PB mode...

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Would take a patch, but doesn't block.

Comment 7

[(no longer active)]

Attachment: Patch (v1)

Patch with an automated test.

Comment 9

[Nochum Sossonko [:Natch]]

This should be somewhat of a priority, as I said in comment 5, if you have a view-source window open during private browsing and then switch out of private browsing mode, the website you were on will still be available via that view-source window...

Note: this is similar to bug 494538.

Comment 10

[Samuel Sidler (old account; do not CC)]

Is mconnor still the right reviewer here?

Comment 11

[(no longer active)]

(In reply to comment #10)
> Is mconnor still the right reviewer here?

He's the only possible reviewer here, given the fact that I'm the PB owner and he's the only peer for that module.

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 369455 [details] [diff] [review]
Patch (v1)


>+
>+      // save view-source windows URIs and close them
>+      let viewSrcWindowsEnum = Cc["@mozilla.org/appshell/window-mediator;1"].
>+                               getService(Ci.nsIWindowMediator).
>+                               getEnumerator("navigator:view-source");
>+      while (viewSrcWindowsEnum.hasMoreElements()) {
>+        let win = viewSrcWindowsEnum.getNext();
>+        if (this._inPrivateBrowsing)
>+          this._viewSrcURIs.push(win.getBrowser().currentURI.spec);

I'd prefer to do the prefix stripping here, rather than after restore.  And if we're just preserving the spec, please name the array URLs, not URIs.  Also, if we get something other than view-source: as the prefix, should we skip?

>+        win.close();
>+      }

I don't know why the _inPrivateBrowsing check is inside of the loop.  Please just stick this code inside of the previous if block, so we only bother with the the enumerator going into private browsing.

>@@ -202,16 +217,41 @@ PrivateBrowsingService.prototype = {

>+
>+        // re-open all view-source windows
>+        let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+                            getService(Ci.nsIWindowWatcher);
>+        this._viewSrcURIs.forEach(function(uri) {
>+          let plainURL = uri;
>+          if (uri.indexOf("view-source:") == 0)
>+            plainURL = uri.substr(12);

As noted, this should be handled on creation of the array.

test looks good.

Comment 13

[(no longer active)]

(In reply to comment #12)
> (From update of attachment 369455 [details] [diff] [review])
> 
> >+
> >+      // save view-source windows URIs and close them
> >+      let viewSrcWindowsEnum = Cc["@mozilla.org/appshell/window-mediator;1"].
> >+                               getService(Ci.nsIWindowMediator).
> >+                               getEnumerator("navigator:view-source");
> >+      while (viewSrcWindowsEnum.hasMoreElements()) {
> >+        let win = viewSrcWindowsEnum.getNext();
> >+        if (this._inPrivateBrowsing)
> >+          this._viewSrcURIs.push(win.getBrowser().currentURI.spec);
> 
> I'd prefer to do the prefix stripping here, rather than after restore.  And if
> we're just preserving the spec, please name the array URLs, not URIs.  Also, if
> we get something other than view-source: as the prefix, should we skip?

Yes, we should.  Addressed this and other comments.

> >+        win.close();
> >+      }
> 
> I don't know why the _inPrivateBrowsing check is inside of the loop.  Please
> just stick this code inside of the previous if block, so we only bother with
> the the enumerator going into private browsing.

No, we do want to close the view source windows which have been opened inside the private browsing mode when switching it off, we just don't want to save their URIs because we're not going to restore them.

> >@@ -202,16 +217,41 @@ PrivateBrowsingService.prototype = {
> 
> >+
> >+        // re-open all view-source windows
> >+        let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"].
> >+                            getService(Ci.nsIWindowWatcher);
> >+        this._viewSrcURIs.forEach(function(uri) {
> >+          let plainURL = uri;
> >+          if (uri.indexOf("view-source:") == 0)
> >+            plainURL = uri.substr(12);
> 
> As noted, this should be handled on creation of the array.

Done.

Landed as <http://hg.mozilla.org/mozilla-central/rev/438b1a88964a>

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 369455 [details] [diff] [review]
Patch (v1)

Doesn't seem like the kind of fix we have to take on the stable branch. 1.9.1 approval denied.