Handle view-source windows in Private Browsing mode

RESOLVED FIXED in Firefox 3.6a1

Status

()

Firefox
Private Browsing
P3
normal
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({privacy})

Trunk
Firefox 3.6a1
privacy
Points:
---
Bug Flags:
blocking-firefox3.5 -
in-testsuite +

Firefox Tracking Flags

(status1.9.1 wontfix)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

10 years ago
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

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

Comment 3

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

Comment 4

10 years ago
Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.
Component: General → Private Browsing
(Assignee)

Updated

10 years ago
QA Contact: general → private.browsing
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...
Flags: blocking-firefox3.1?
Would take a patch, but doesn't block.
Flags: blocking-firefox3.1? → blocking-firefox3.1-
(Assignee)

Updated

9 years ago
Whiteboard: [PB-P2]

Updated

9 years ago
Priority: -- → P3
Target Milestone: --- → Firefox 3.2a1
(Assignee)

Comment 7

9 years ago
Created attachment 369455 [details] [diff] [review]
Patch (v1)

Patch with an automated test.
Attachment #369455 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Whiteboard: [PB-P2]

Updated

9 years ago
Duplicate of this bug: 494781
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.
Flags: wanted1.9.1.x?
Flags: blocking-firefox3.6?
Is mconnor still the right reviewer here?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Whiteboard: [needs r=mconnor]
(Assignee)

Comment 11

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

Updated

9 years ago
Attachment #369455 - Flags: review?(mconnor) → review+
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.

Updated

9 years ago
Whiteboard: [needs r=mconnor]
(Assignee)

Comment 13

9 years ago
(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>
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: blocking-firefox3.6? → in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Attachment #369455 - Flags: approval1.9.1.3?
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.
Attachment #369455 - Flags: approval1.9.1.3? → approval1.9.1.3-
status1.9.1: ? → wontfix
(Assignee)

Updated

9 years ago
Duplicate of this bug: 527671
You need to log in before you can comment on or make changes to this bug.