Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Private browsing service executes transition even when no mode switch required

RESOLVED FIXED in Firefox 8

Status

()

Firefox
Private Browsing
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
pb.privateBrowsingEnabled = false;

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

Comment 1

6 years ago
Created attachment 527398 [details] [diff] [review]
patch v1
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+
(Assignee)

Comment 3

6 years ago
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 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.
(Assignee)

Updated

6 years ago
Blocks: 650280
(Assignee)

Comment 5

6 years ago
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.
Attachment #527398 - Attachment is obsolete: true
Attachment #527419 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Failed on try, investigating.
Attachment #528222 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Depends on: 618188, 650573
(Assignee)

Updated

6 years ago
Blocks: 658832
(Assignee)

Comment 7

6 years ago
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).
Attachment #549730 - Flags: review?(ehsan)
(Assignee)

Comment 8

6 years ago
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.
Attachment #549731 - Flags: review?(paul)
Attachment #549730 - Flags: review?(ehsan) → review+
Attachment #549731 - Flags: review?(paul) → review+
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/1a9f0823fdba
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/mozilla-central/rev/1a9f0823fdba
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8

Updated

6 years ago
Blocks: 658738

Updated

6 years ago
No longer blocks: 658738

Updated

6 years ago
Blocks: 658738
Any idea why this patch would have resolved browser.xul leaks (bug 658738 comment 72)?
(Assignee)

Comment 12

6 years ago
(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.