651643 - Private browsing service executes transition even when no mode switch required

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

pb.privateBrowsingEnabled = false;

This starts and ends a transition even if private browsing is currently disabled.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[(no longer active)]

Comment on attachment 527398 [details] [diff] [review]
patch v1

I'm assuming that all you did here was add that if statement before the try block, and remove the reverse if check from inside the try block and reindent, right?  r=me assuming that!

Thanks for your patch.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1 (without white space changes)

(In reply to comment #2)
> I'm assuming that all you did here was add that if statement before the try
> block, and remove the reverse if check from inside the try block and reindent,
> right?  r=me assuming that!

I attached the patch without the whitespace changes - that shows the changes a bit cleaner :)

Comment 4

[(no longer active)]

Comment on attachment 527419 [details] [diff] [review]
patch v1 (without white space changes)

>+      this._currentStatus = STATE_TRANSITION_STARTED;
>         this._ensureCanCloseWindows();

Actually I think these two lines should be swapped, since _ensureCanCloseWindows can lead to an early return as well.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

(In reply to comment #4)
> >+      this._currentStatus = STATE_TRANSITION_STARTED;
> >         this._ensureCanCloseWindows();
> 
> Actually I think these two lines should be swapped, since
> _ensureCanCloseWindows can lead to an early return as well.

True, fixed.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 528222 [details] [diff] [review]
patch for checkin

Failed on try, investigating.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

This patch revealed some errors so I had to fix some tests, I didn't touch nsPrivateBrowsingService.js.

* ui.js - When a session is restored (e.g. when switching pb mode) it's possible that UI.setActive() is called and a tabItem is passed that temporarily has no parent. So we need to check for tabItem.parent or else GroupItems.setActiveGroupItem() will fail.

* browser_tabview_privatebrowsing.js - When cleaning up after this test we're removing the second tab. We need to make sure that this tab is not selected or else tabview will be shown again (and causes the next test to fail).

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2 (session store tests)

This patch makes some session restore tests fail because they rely on Panorama listeners getting called _after_ the test listeners (for SSWindowStateReady). So I rewrote those tests to run in a separate window where Panorama isn't loaded, yet.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/1a9f0823fdba

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/1a9f0823fdba

Comment 11

[Dão Gottwald [:dao]]

Any idea why this patch would have resolved browser.xul leaks (bug 658738 comment 72)?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #11)
> Any idea why this patch would have resolved browser.xul leaks (bug 658738
> comment 72)?

I don't have a clue. Maybe Ehsan has some thoughts?

Comment 13

[(no longer active)]

(In reply to comment #12)
> (In reply to Dão Gottwald [:dao] from comment #11)
> > Any idea why this patch would have resolved browser.xul leaks (bug 658738
> > comment 72)?
> 
> I don't have a clue. Maybe Ehsan has some thoughts?

No, not really...