Entering the Private Browsing mode may not always empty the download manager list

VERIFIED FIXED in mozilla1.9.1b2

Status

()

Toolkit
Downloads API
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.1b2
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
Created attachment 347136 [details] [diff] [review]
Patch (v1)

(From bug #248970 comment #499)
> > >@@ -504,16 +517,24 @@ let gDownloadObserver = {
> > >+      case "private-browsing":
> > >+        if (aData == "enter" || aData == "exit") {
> > >+          setTimeout(function() {
> > >+            initStmt();
> > >+            buildDownloadList(true);
> > >+          }, 0);
> > >+        }
> > Why are we doing this in a setTimeout?  If there is a good reason, we should
> > have a comment explaining why.
> 
> Hmmm, I couldn't remember why I did that, but I think it should've been a
> mistake.  I removed the setTimeout call and tested the patch again to make sure
> everything's working properly.

Actually I just remembered why I was doing this.  When processing private-browsing in the UI, the private-browsing notification might not be received by the download manager yet, so this code may re-init the statement to the same on-disk database connection, which will leave the download manager UI in an inconsistent state (it will continue to query the on-disk DB, while the download manager service is using the in-memory DB).  The patch includes a comment explaining the need for the setTimeout call.

This is a serious problem.  We need to have this for Beta 2.  Shawn: a quick review is appreciated here!  :-)
Flags: in-litmus?
Flags: blocking1.9.1?
Attachment #347136 - Flags: review?(sdwilsh)
Attachment #347136 - Flags: approval1.9.1b2?
I normally wouldn't block the beta for this sort of bug, but if the case is that we need to see how this sort of code actually works "in the field" then I agree that we should.
Flags: blocking1.9.1? → blocking1.9.1+
Comment on attachment 347136 [details] [diff] [review]
Patch (v1)

r=sdwilsh
Attachment #347136 - Flags: review?(sdwilsh) → review+
Comment on attachment 347136 [details] [diff] [review]
Patch (v1)

a=beltzner, though redundant as this is a b2 blocker
Attachment #347136 - Flags: approval1.9.1b2? → approval1.9.1b2+
In order to write a test case for this in Litmus, I need to clarify if the download manager info from my regular session will be cleared when I enter PB mode.
(In reply to comment #4)
> In order to write a test case for this in Litmus, I need to clarify if the
> download manager info from my regular session will be cleared when I enter PB
> mode.
I'm not sure if a litmus case is useful since the ordering can be random here.
Keywords: checkin-needed
Whiteboard: [has approval]
(Assignee)

Comment 6

10 years ago
I meant to check this in tonight, but the tree is still closed.  :-(  Will try again tomorrow, in the mean time feel free to check this in if you catch it on an open tree sooner than me.

(In reply to comment #4)
> In order to write a test case for this in Litmus, I need to clarify if the
> download manager info from my regular session will be cleared when I enter PB
> mode.

Yes, it should be cleared.  Two Litmus tests are probably needed here, one with the DM window closed before initiating the PB mode, and another one while it's open to observe that the list is indeed cleared upon mode switch.  On leaving the PB mode, the private download list (if any) should be cleared and replaced with the regular downloads list.

(In reply to comment #5)
> I'm not sure if a litmus case is useful since the ordering can be random here.

Hmm, what do you mean?
(Assignee)

Comment 7

10 years ago
http://hg.mozilla.org/mozilla-central/rev/8e91e34f923c
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has approval]
verified fixed using:
*  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081113 Minefield/3.1b2pre
* Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081113 Minefield/3.1b2pre
*  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081113 Minefield/3.1b2pre

There is one minor bug that I can see that occurs on Windows XP when you have an active download in progress, but I will file that separately.
Status: RESOLVED → VERIFIED
https://litmus.mozilla.org/show_test.cgi?id=7434 added to Litmus. I don't think we need a test for both states, and on Mac you cannot have the DM window open and launch a PB session - you have to close it first.
Flags: in-litmus? → in-litmus+
(Assignee)

Comment 10

10 years ago
(In reply to comment #8)
> There is one minor bug that I can see that occurs on Windows XP when you have
> an active download in progress, but I will file that separately.

Did you file the bug you mentioned here?

(In reply to comment #9)
> https://litmus.mozilla.org/show_test.cgi?id=7434 added to Litmus.

Seems like part of comment 6 has been mistakenly pasted at the end of that test case.

> I don't think we need a test for both states,

I think we do, because if the DM window is opened along the way, all the list entries are re-initialized.  So the test with the DM window closed would make sure that the back-end and the front-end links are working correctly, and the test with the DM window open would make sure that the fix in this bug is working correctly.

> and on Mac you cannot have the DM window open
> and launch a PB session - you have to close it first.

Oh, forgive my Mac ignorance, but why is that?
Ehsan: I filed Bug 464800 to address the issue I see on both XP/Vista, which is relatively minor.

I adjusted the Litmus test case you noted in Comment 10 and will add another one to cover the case of the DM being open during the process.

Probably someone on the Mac dev team such as Josh Aas or Steven Michaud could better answer the question as to why the DM has to be closed in order to toggle that pref under Tools.
It turns out that the only time on Mac you cannot switch the PB toggle is when the DM window is in front, when it is minimized or behind the main browser window you can toggle the setting fine.
Keywords: fixed1.9.1
Ehsan, is it the same or related to what I've seen on bug 460608?

Marcia has already verified this patch on 1.9.1. Adjusting the keyword.
Keywords: fixed1.9.1 → verified1.9.1
(Assignee)

Comment 14

9 years ago
(In reply to comment #12)
> It turns out that the only time on Mac you cannot switch the PB toggle is when
> the DM window is in front, when it is minimized or behind the main browser
> window you can toggle the setting fine.

Marcia, can you file a bug on this please, with STR and preferably error console output?  Thanks!
(Assignee)

Comment 15

9 years ago
(In reply to comment #13)
> Ehsan, is it the same or related to what I've seen on bug 460608?

What do you mean?
(In reply to comment #15)
> (In reply to comment #13)
> > Ehsan, is it the same or related to what I've seen on bug 460608?
> 
> What do you mean?

In bug 460608 I haven't taken a look inside the downloads.sqlite. Leaving the PB mode hasn't removed these downloads from the download manager. So probably its the same issue as reported by this bug. It was just a side-note.
(Assignee)

Comment 17

9 years ago
I don't think so.  This was not a bug in the back-end -- it was merely a display issue.  In other words, no downloads were stuck in downloads.sqlite -- only the UI's list wasn't being updated properly.
You need to log in before you can comment on or make changes to this bug.