Closed Bug 529667 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox :: Private Browsing, defect, P1)

3.6 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: whimboo, Assigned: ehsan)

References

Details

(Keywords: regression, verified1.9.2)

Attachments

(1 file, 1 obsolete file)

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.
Flags: blocking-firefox3.6?
This is embarrassing.  I have a patch, working on a test...
Assignee: nobody → ehsan.akhgari
Severity: normal → major
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #413211 - Flags: review?(vladimir)
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Priority: -- → P1
Whiteboard: [can land]
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?
Attached patch Patch (v1.1)Splinter Review
(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.
Attachment #413211 - Attachment is obsolete: true
Attachment #413348 - Flags: review?(vladimir)
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.
http://hg.mozilla.org/mozilla-central/rev/c6991463f32a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → Firefox 3.7a1
(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?
You could do what browser_bug495058.js does.
Whiteboard: [can land 1.9.2]
Whiteboard: [can land 1.9.2]
Depends on: 529922
(In reply to comment #8)
> You could do what browser_bug495058.js does.

Filed bug 529922 for that (with a patch.)
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
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.