Last Comment Bug 749187 - Ensure new tabs and windows opened in private browsing mode have docshells set accordingly
: Ensure new tabs and windows opened in private browsing mode have docshells se...
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: :Ehsan Akhgari
:
: :Ehsan Akhgari
Mentors:
Depends on:
Blocks: PBnGen 748802
  Show dependency treegraph
 
Reported: 2012-04-26 07:34 PDT by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-05-22 11:39 PDT (History)
6 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
Patch (v1) (15.70 KB, patch)
2012-04-26 11:14 PDT, :Ehsan Akhgari
gavin.sharp: review-
Details | Diff | Splinter Review
Patch (v2) (13.15 KB, patch)
2012-04-26 13:38 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v3) (13.00 KB, patch)
2012-04-26 13:47 PDT, :Ehsan Akhgari
gavin.sharp: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-26 07:34:47 PDT
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] (on vacation until Dec 5) 2012-04-26 07:36:14 PDT
and by Tim I really mean Raymond.
Comment 2 :Ehsan Akhgari 2012-04-26 09:15:43 PDT
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 :Ehsan Akhgari 2012-04-26 11:14:38 PDT
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.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-26 11:57:08 PDT
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 :Ehsan Akhgari 2012-04-26 13:33:56 PDT
(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.
Comment 6 Dão Gottwald [:dao] 2012-04-26 13:36:51 PDT
(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 :Ehsan Akhgari 2012-04-26 13:38:23 PDT
Created attachment 618787 [details] [diff] [review]
Patch (v2)
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-26 13:39:07 PDT
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: gavin@gavinsharp.com] 2012-04-26 13:39:32 PDT
(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] 2012-04-26 13:41:25 PDT
(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).
Comment 11 :Ehsan Akhgari 2012-04-26 13:47:46 PDT
Created attachment 618788 [details] [diff] [review]
Patch (v3)
Comment 12 :Ehsan Akhgari 2012-05-02 16:40:16 PDT
Gavin: ping?
Comment 13 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-05-12 03:26:02 PDT
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 :Ehsan Akhgari 2012-05-14 12:36:46 PDT
Josh, do you wanna review this yourself?  :-)
Comment 15 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-05-14 14:57:57 PDT
Nope, I don't think I'm a good choice.
Comment 16 :Ehsan Akhgari 2012-05-16 12:00:44 PDT
Gavin, can you please provide a review here based on our IRC discussion a few days ago?  Thanks!
Comment 17 :Ehsan Akhgari 2012-05-16 12:55:30 PDT
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: gavin@gavinsharp.com] 2012-05-16 13:48:23 PDT
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 20 :Ehsan Akhgari 2012-05-16 14:40:23 PDT
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] 2012-05-17 03:18:28 PDT
https://hg.mozilla.org/mozilla-central/rev/d4068b54381a
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-18 15:51:42 PDT
Comment on attachment 618788 [details] [diff] [review]
Patch (v3)

approving, regression fix.

Note You need to log in before you can comment on or make changes to this bug.