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

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[(no longer active)]

Attachment: 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!  :-)

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 347136 [details] [diff] [review]
Patch (v1)

r=sdwilsh

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347136 [details] [diff] [review]
Patch (v1)

a=beltzner, though redundant as this is a b2 blocker

Comment 4

[Marcia Knous [:marcia]]

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.

Comment 5

[Shawn Wilsher :sdwilsh]

(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.

Comment 6

[(no longer active)]

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?

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8e91e34f923c

Comment 8

[Marcia Knous [:marcia]]

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.

Comment 9

[Marcia Knous [:marcia]]

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.

Comment 10

[(no longer active)]

(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?

Comment 11

[Marcia Knous [:marcia]]

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.

Comment 12

[Marcia Knous [:marcia]]

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.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+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.

Comment 14

[(no longer active)]

(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!

Comment 15

[(no longer active)]

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

What do you mean?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 17

[(no longer active)]

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.