Ensure new tabs and windows opened in private browsing mode have docshells set accordingly

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Private Browsing
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

({regression})

Trunk
Firefox 15
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14+ fixed, firefox15+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
This bug is for investigation of the issue, and a fix if it actually does exist. Please follow up on Tim and Dao's comments in bug 748802 that suggest that new windows and tabs opened in global private browsing mode may not inherit the correct docshell privacy values, leading to incorrect privateWindow results.
(Reporter)

Comment 1

5 years ago
and by Tim I really mean Raymond.

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
We have a bunch of stuff using this flag in Firefox 14, they will break if you open a new window in PB mode.
Assignee: nobody → ehsan
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox14: --- → +
tracking-firefox15: --- → +
Created attachment 618727 [details] [diff] [review]
Patch (v1)

OK, this patch adds the necessary handling for newly opened windows.  This is a pretty serious problem, and we definitely want to take this for Firefox 14.
Attachment #618727 - Flags: review?(gavin.sharp)
Comment on attachment 618727 [details] [diff] [review]
Patch (v1)

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

>   get privateWindow() {

>+    return window.QueryInterface(Ci.nsIInterfaceRequestor)
>+                 .getInterface(nsIWebNavigation)

Please don't use "nsIWebNavigation" (it shouldn't exist), and continue using Ci.nsIWebNavigation.

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js

>-      if (!this._quitting) {
>-        var windowsEnum = Services.wm.getEnumerator("navigator:browser");
>-        while (windowsEnum.hasMoreElements()) {

>+    var windowsEnum = Services.wm.getEnumerator("navigator:browser");
>+    while (windowsEnum.hasMoreElements()) {

I don't understand why you're removing the !quitting check or moving this code, it seems unrelated to this patch.

>+      case "domwindowopened":
>+        let aWindow = aSubject;
>+        let self = this;
>+        aWindow.QueryInterface(Ci.nsIDOMWindow);

Technically you probably want to QI to nsIDOMEventTarget? Applies to the test too.

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_ui.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_ui.js

>+  function testNewWindow(aCallback, expected) {
>+    let observer1 = {
>+      observe: function(aSubject, aTopic, aData) {

function observer1(aSubject, aTopic, aData)

>+          SimpleTest.executeSoon(function() {

you don't need the "SimpleTest."

>+            let ui = XPCNativeWrapper.unwrap(aSubject).gPrivateBrowsingUI;

This unwrap shouldn't be necessary either.
Attachment #618727 - Flags: review?(gavin.sharp) → review-
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> Comment on attachment 618727 [details] [diff] [review]
> Patch (v1)
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >   get privateWindow() {
> 
> >+    return window.QueryInterface(Ci.nsIInterfaceRequestor)
> >+                 .getInterface(nsIWebNavigation)
> 
> Please don't use "nsIWebNavigation" (it shouldn't exist), and continue using
> Ci.nsIWebNavigation.

Will fix.

> >diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
> 
> >-      if (!this._quitting) {
> >-        var windowsEnum = Services.wm.getEnumerator("navigator:browser");
> >-        while (windowsEnum.hasMoreElements()) {
> 
> >+    var windowsEnum = Services.wm.getEnumerator("navigator:browser");
> >+    while (windowsEnum.hasMoreElements()) {
> 
> I don't understand why you're removing the !quitting check or moving this
> code, it seems unrelated to this patch.

Because not doing this will cause the "last-pb-context-exited" event to not be fired when quitting, which is bad.  I guess this is technically not related to this bug, and I can move it to another bug if you want me to.

> >+      case "domwindowopened":
> >+        let aWindow = aSubject;
> >+        let self = this;
> >+        aWindow.QueryInterface(Ci.nsIDOMWindow);
> 
> Technically you probably want to QI to nsIDOMEventTarget? Applies to the
> test too.

Ah, right.

> >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_ui.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_ui.js
> 
> >+  function testNewWindow(aCallback, expected) {
> >+    let observer1 = {
> >+      observe: function(aSubject, aTopic, aData) {
> 
> function observer1(aSubject, aTopic, aData)

That's what I did originally, but removeObserver(arguments.callee) would not work correctly, which is why I switched to this more verbose code.
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > >+      case "domwindowopened":
> > >+        let aWindow = aSubject;
> > >+        let self = this;
> > >+        aWindow.QueryInterface(Ci.nsIDOMWindow);
> > 
> > Technically you probably want to QI to nsIDOMEventTarget? Applies to the
> > test too.
> 
> Ah, right.

Under which circumstances is this actually needed?
Created attachment 618787 [details] [diff] [review]
Patch (v2)
Attachment #618727 - Attachment is obsolete: true
Attachment #618787 - Flags: review?(gavin.sharp)
Oh, I forgot to mention that you also shouldn't use arguments.callee :)

Services.obs.addObserver(function obs() {
  Services.obs.removeObserver(obs, "domwindowopened");
}, "domwindowopened", false);

should work fine?
(In reply to Dão Gottwald [:dao] from comment #6)
> Under which circumstances is this actually needed?

I don't know. Probably none?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > Under which circumstances is this actually needed?
> 
> I don't know. Probably none?

That's what I thought. I always omit the QueryInterface (including for nsIDOMWindow).
Created attachment 618788 [details] [diff] [review]
Patch (v3)
Attachment #618788 - Flags: review?(gavin.sharp)
Attachment #618787 - Attachment is obsolete: true
Attachment #618787 - Flags: review?(gavin.sharp)
Gavin: ping?
(Reporter)

Comment 13

5 years ago
Gavin, this is a serious regression in behaviour. Can you prioritize this review accordingly, or pass it off to someone else who can review it quickly?
Josh, do you wanna review this yourself?  :-)
(Reporter)

Comment 15

5 years ago
Nope, I don't think I'm a good choice.
Gavin, can you please provide a review here based on our IRC discussion a few days ago?  Thanks!
Comment on attachment 618788 [details] [diff] [review]
Patch (v3)

Review of attachment 618788 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
@@ +237,5 @@
> +    var windowsEnum = Services.wm.getEnumerator("navigator:browser");
> +    while (windowsEnum.hasMoreElements()) {
> +      var window = windowsEnum.getNext();
> +      this._setPerWindowPBFlag(window, this._inPrivateBrowsing);
> +    }

The reason that this hunk is necessary is that if we don't set the PB flag on some docshells when quitting, the last-pb-exited event may not be dispatched properly, which could prevent some modules which depend on it from cleaning up properly.
Comment on attachment 618788 [details] [diff] [review]
Patch (v3)

none of the QueryInterface(Ci.nsIDOM*) calls in this patch (both code and test) are necessary, you can remove them.
Attachment #618788 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4068b54381a
status-firefox15: affected → fixed
Flags: in-testsuite+
Target Milestone: --- → Firefox 15
Comment on attachment 618788 [details] [diff] [review]
Patch (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Many per window PB bugs.
User impact if declined: Serious regression in PB mode.
Testing completed (on m-c, etc.): Using automated testing.
Risk to taking this patch (and alternatives if risky): Minimal.
String or UUID changes made by this patch: None.
Attachment #618788 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d4068b54381a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Comment on attachment 618788 [details] [diff] [review]
Patch (v3)

approving, regression fix.
Attachment #618788 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: regression
http://hg.mozilla.org/releases/mozilla-aurora/rev/ce3cc54fe3bb
status-firefox14: affected → fixed
You need to log in before you can comment on or make changes to this bug.