529667 - Stopping PB mode after opening a new window when PB is active is not possible

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091118 Minefield/3.7a1pre ID:20091118032442

Opening a new window while staying in Private Browsing mode will disable the "Stop Private Browsing" menu entry for that window. You have you switch to the origin window to be able to.

Steps:
1. Enter PB mode
2. Open a new window
3. Open the tools menu

In step 3 you will see that there is no way to stop the Private Browsing mode.

Given by ehsan this is a regression from bug 526194.

Comment 1

[(no longer active)]

This is embarrassing.  I have a patch, working on a test...

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Comment 3

[Dão Gottwald [:dao]]

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

>+  let observer = {
>+    observe: function(aSubject, aTopic, aData) {
>+      if (aTopic == "domwindowclosed") {
>+        ww.unregisterNotification(this);
>+        pb.privateBrowsingEnabled = false;
>+        finish();
>+      }
>+    }
>+  };
>+
>+  ww.registerNotification(observer);
>+  let win = OpenBrowserWindow();
>+  win.addEventListener("load", function() {
>+    executeSoon(function() {
...
>+      win.close();
>+    });
>+  }, false);
>+}

This pattern has previously caused random failures, as you may be getting a domwindowclosed notification from a previous test. Please either make sure that this is for the window you're interested in or don't use domwindowclosed. (I don't see offhand why it's used in this test.)

>+  win.addEventListener("load", function() {
>+    executeSoon(function() {
>+      let cmd = win.document.getElementById("Tools:PrivateBrowsing");
>+      ok(!cmd.hasAttribute("disabled"),
>+         "The Private Browsing command in a new window should be enabled");

Since gPrivateBrowsingUI.init() is called from delayedStartup, does your patch actually make a difference for this test?

Comment 4

[(no longer active)]

Attachment: Patch (v1.1)

(In reply to comment #3)
> (From update of attachment 413211 [details] [diff] [review])
> >+  let observer = {
> >+    observe: function(aSubject, aTopic, aData) {
> >+      if (aTopic == "domwindowclosed") {
> >+        ww.unregisterNotification(this);
> >+        pb.privateBrowsingEnabled = false;
> >+        finish();
> >+      }
> >+    }
> >+  };
> >+
> >+  ww.registerNotification(observer);
> >+  let win = OpenBrowserWindow();
> >+  win.addEventListener("load", function() {
> >+    executeSoon(function() {
> ...
> >+      win.close();
> >+    });
> >+  }, false);
> >+}
> 
> This pattern has previously caused random failures, as you may be getting a
> domwindowclosed notification from a previous test. Please either make sure that
> this is for the window you're interested in or don't use domwindowclosed. (I
> don't see offhand why it's used in this test.)

I was worried that not doing this might introduce some random oranges, but with bug 528451 this should no longer be an issue.

> >+  win.addEventListener("load", function() {
> >+    executeSoon(function() {
> >+      let cmd = win.document.getElementById("Tools:PrivateBrowsing");
> >+      ok(!cmd.hasAttribute("disabled"),
> >+         "The Private Browsing command in a new window should be enabled");
> 
> Since gPrivateBrowsingUI.init() is called from delayedStartup, does your patch
> actually make a difference for this test?

Actually it does, but I see your point.  I guess I was relying on the order of load event handlers for this to matter, but in this version of the patch I'm using two executeSoon calls to make sure the second one happens after delayedStartup.

Comment 5

[Dão Gottwald [:dao]]

Note that bug 512645 is on trunk but not on branch, so while nesting executeSoon might put you after delayedStartup on trunk, it might not do so on branch.

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c6991463f32a

Comment 7

[(no longer active)]

(In reply to comment #5)
> Note that bug 512645 is on trunk but not on branch, so while nesting
> executeSoon might put you after delayedStartup on trunk, it might not do so on
> branch.

Is there any workaround for that?

Comment 8

[Dão Gottwald [:dao]]

You could do what browser_bug495058.js does.

Comment 9

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a7b148a1250b

Comment 10

[(no longer active)]

(In reply to comment #8)
> You could do what browser_bug495058.js does.

Filed bug 529922 for that (with a patch.)

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk and 1.9.2 with builds on OS X and Windows like:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091120 Minefield/3.7a1pre ID:20091120030847

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b4pre) Gecko/20091120 Namoroka/3.6b4pre ID:20091120033655