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

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Josh Matthews [:jdm]]

and by Tim I really mean Raymond.

Comment 2

[(no longer active)]

We have a bunch of stuff using this flag in Firefox 14, they will break if you open a new window in PB mode.

Comment 3

[(no longer active)]

Attachment: 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.

Comment 4

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

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.

Comment 5

[(no longer active)]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 6

[Dão Gottwald [:dao]]

(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?

Comment 7

[(no longer active)]

Attachment: Patch (v2)

Comment 8

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

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?

Comment 9

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

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

I don't know. Probably none?

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] 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).

Comment 11

[(no longer active)]

Attachment: Patch (v3)

Comment 12

[(no longer active)]

Gavin: ping?

Comment 13

[Josh Matthews [:jdm]]

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?

Comment 14

[(no longer active)]

Josh, do you wanna review this yourself?  :-)

Comment 15

[Josh Matthews [:jdm]]

Nope, I don't think I'm a good choice.

Comment 16

[(no longer active)]

Gavin, can you please provide a review here based on our IRC discussion a few days ago?  Thanks!

Comment 17

[(no longer active)]

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 18

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

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.

Comment 19

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4068b54381a

Comment 20

[(no longer active)]

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.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d4068b54381a

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 618788 [details] [diff] [review]
Patch (v3)

approving, regression fix.

Comment 23

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/ce3cc54fe3bb