Closed Bug 651643 Opened 9 years ago Closed 9 years ago

Private browsing service executes transition even when no mode switch required

Categories

(Firefox :: Private Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 8

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(2 files, 3 obsolete files)

pb.privateBrowsingEnabled = false;

This starts and ends a transition even if private browsing is currently disabled.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #527398 - Flags: review?(ehsan)
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.
Attachment #527398 - Flags: review?(ehsan) → review+
(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 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.
Blocks: 650280
Attached patch patch for checkin (obsolete) — Splinter Review
(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.
Attachment #527398 - Attachment is obsolete: true
Attachment #527419 - Attachment is obsolete: true
Comment on attachment 528222 [details] [diff] [review]
patch for checkin

Failed on try, investigating.
Attachment #528222 - Attachment is obsolete: true
Depends on: 618188, 650573
Blocks: 658832
Attached patch patch v2Splinter Review
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).
Attachment #549730 - Flags: review?(ehsan)
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.
Attachment #549731 - Flags: review?(paul)
Attachment #549730 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/1a9f0823fdba
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Blocks: bc-leaks
No longer blocks: bc-leaks
Blocks: bc-leaks
Any idea why this patch would have resolved browser.xul leaks (bug 658738 comment 72)?
(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?
(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...
You need to log in before you can comment on or make changes to this bug.