Last Comment Bug 651643 - Private browsing service executes transition even when no mode switch required
: Private browsing service executes transition even when no mode switch required
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 618188 650573
Blocks: 650280 bc-leaks 658832
  Show dependency treegraph
 
Reported: 2011-04-20 14:55 PDT by Tim Taubert [:ttaubert]
Modified: 2011-08-09 14:21 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (3.43 KB, patch)
2011-04-20 15:07 PDT, Tim Taubert [:ttaubert]
ehsan: review+
Details | Diff | Splinter Review
patch v1 (without white space changes) (2.32 KB, patch)
2011-04-20 16:34 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch for checkin (3.08 KB, patch)
2011-04-25 16:48 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (5.12 KB, patch)
2011-08-01 00:24 PDT, Tim Taubert [:ttaubert]
ehsan: review+
Details | Diff | Splinter Review
patch v2 (session store tests) (5.95 KB, patch)
2011-08-01 00:28 PDT, Tim Taubert [:ttaubert]
paul: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-04-20 14:55:01 PDT
pb.privateBrowsingEnabled = false;

This starts and ends a transition even if private browsing is currently disabled.
Comment 1 Tim Taubert [:ttaubert] 2011-04-20 15:07:45 PDT
Created attachment 527398 [details] [diff] [review]
patch v1
Comment 2 :Ehsan Akhgari (away Aug 1-5) 2011-04-20 15:38:15 PDT
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] 2011-04-20 16:34:18 PDT
Created attachment 527419 [details] [diff] [review]
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 :Ehsan Akhgari (away Aug 1-5) 2011-04-21 08:07:08 PDT
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] 2011-04-25 16:48:52 PDT
Created attachment 528222 [details] [diff] [review]
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] 2011-04-26 02:26:28 PDT
Comment on attachment 528222 [details] [diff] [review]
patch for checkin

Failed on try, investigating.
Comment 7 Tim Taubert [:ttaubert] 2011-08-01 00:24:57 PDT
Created attachment 549730 [details] [diff] [review]
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] 2011-08-01 00:28:20 PDT
Created attachment 549731 [details] [diff] [review]
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] 2011-08-03 18:52:21 PDT
http://hg.mozilla.org/integration/fx-team/rev/1a9f0823fdba
Comment 10 Tim Taubert [:ttaubert] 2011-08-04 05:14:50 PDT
http://hg.mozilla.org/mozilla-central/rev/1a9f0823fdba
Comment 11 Dão Gottwald [:dao] 2011-08-09 03:31:56 PDT
Any idea why this patch would have resolved browser.xul leaks (bug 658738 comment 72)?
Comment 12 Tim Taubert [:ttaubert] 2011-08-09 07:21:54 PDT
(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 :Ehsan Akhgari (away Aug 1-5) 2011-08-09 14:21:53 PDT
(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...

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