463021 - Private browsing tests use timeouts

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Dave Townsend [:mossop]]

The private browsing browser tests use timeout for no apparent reason. These slow down the test run and can cause the test to perform unpredictably under load. The timeouts should be removed or comments added explaining the necessity.

Comment 1

[(no longer active)]

I'll investigate this issue.

Comment 2

[(no longer active)]

One of the tests which is using a timeout (well, a timer to be precise) is the download manager test:

<http://hg.mozilla.org/mozilla-central/file/b4fa4f973642/toolkit/components/downloads/test/unit/test_privatebrowsing.js#l164>

I think this is inevitable there, because of the fact that we want to give the download manager enough time and see if it continues downloading the file (which it shouldn't).  Shawn: if you feel there is a better way to check this instead of a timeout, please comment here.

Comment 3

[(no longer active)]

Apart from the bugs marked as dependencies of this one, I do not know of any other private browsing test which uses timeouts.  If I have missed something, please file a new bug and make it block this one.

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> I think this is inevitable there, because of the fact that we want to give the
> download manager enough time and see if it continues downloading the file
> (which it shouldn't).  Shawn: if you feel there is a better way to check this
> instead of a timeout, please comment here.
http://hg.mozilla.org/mozilla-central/file/b4fa4f973642/toolkit/components/downloads/test/unit/test_privatebrowsing.js#l160

The question is what should happen, and why can't we test for it?

Comment 5

[(no longer active)]

(In reply to comment #4)
> http://hg.mozilla.org/mozilla-central/file/b4fa4f973642/toolkit/components/downloads/test/unit/test_privatebrowsing.js#l160
> 
> The question is what should happen, and why can't we test for it?

The expected thing here is for the download not to finish.  And I'm testing for it by waiting for some time to see if the download is finished during that time.  The flakiness here is that theoretically if the machine is excessively slow, it may be possible that something is wrong and the download actually continues to progress, but doesn't finish in the time we spend watching it (3 seconds).  This _could_ lead to false negatives.  I couldn't think of a better way to test this, so please let me know if you have any ideas.

Comment 6

[Shawn Wilsher :sdwilsh]

It doesn't finish and it gets removed, right?  If that is the case, you can listen to an observer topic to know when a download gets removed.

Comment 7

[(no longer active)]

(In reply to comment #6)
> It doesn't finish and it gets removed, right?

Yes.

> If that is the case, you can
> listen to an observer topic to know when a download gets removed.

Actually, what we do is pause all downloads, and then remove them all.  nsDownloadManager::PauseAllDownloads doesn't send any notifications, and nsDownloadManager::RemoveAllDownloads only sends dl-cancel when the download is not already paused, right?

Maybe it would be a good idea to add a "dl-pause" observer notification and use that inside the test.  What do you think?

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Actually, what we do is pause all downloads, and then remove them all. 
> nsDownloadManager::PauseAllDownloads doesn't send any notifications, and
> nsDownloadManager::RemoveAllDownloads only sends dl-cancel when the download is
> not already paused, right?
RemoveAllDownloads only seems to stop all active downloads and that's it.  I don't see it removing anything from the database.

> Maybe it would be a good idea to add a "dl-pause" observer notification and use
> that inside the test.  What do you think?
No.  I've had extension authors ask for that too, and have said the same thing.  If you need finer grained feedback on downloads, that is what the download listener is for.

The observer topic I was refering to is in removeDownload and cleanup:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1687

Comment 9

[(no longer active)]

(In reply to comment #8)
> RemoveAllDownloads only seems to stop all active downloads and that's it.  I
> don't see it removing anything from the database.

We do not want to (and certainly do not) remove anything from the database.  Upon switching to the private mode, we only remove active downloads, and close the disk-based DB connection and open a new memory-based DB connection.

> No.  I've had extension authors ask for that too, and have said the same thing.
>  If you need finer grained feedback on downloads, that is what the download
> listener is for.
> 
> The observer topic I was refering to is in removeDownload and cleanup:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1687

Since we don't actually call nsDownloadManager::RemoveDownload, I guess we can't use this notification in the test as well, because it won't ever be fired.

Comment 10

[Shawn Wilsher :sdwilsh]

Well, does leaving private browsing mode close a download manager window if it's open?

Comment 11

[Marcia Knous [:marcia]]

On Mac, it does not if the window is in the background. It simply clears the data, but the DM window is still open in the background. If the DM is open in the foreground you cannot operating the PB toggle.

(In reply to comment #10)
> Well, does leaving private browsing mode close a download manager window if
> it's open?

Comment 12

[(no longer active)]

(In reply to comment #10)
> Well, does leaving private browsing mode close a download manager window if
> it's open?

Like Marcia mention, no it does not.  And it's not supposed to do so...

Comment 13

[Shawn Wilsher :sdwilsh]

Then you should be dispatching the topic so the UI updates accordingly.

Comment 14

[(no longer active)]

(In reply to comment #13)
> Then you should be dispatching the topic so the UI updates accordingly.

Hmm, the UI needs to reload the whole list anyway, and it does that, so there's no need to dispatch this topic as well for the UI to be updated.

Comment 15

[Shawn Wilsher :sdwilsh]

How does the UI currently know that it needs to update?

Comment 16

[(no longer active)]

(In reply to comment #15)
> How does the UI currently know that it needs to update?

It catches the private-browsing topic, resets the DB statement it uses, and re-inits the list.

Comment 17

[(no longer active)]

Attachment: Don't use a timeout in the download manager test

Shawn, what do you think about this approach?  After some thinking, I figured that it should be enough to check after entering the private browsing mode that the download has been paused, and it's not available in the active downloads list.  This patch does that, and removes the timeout and the machinery designed to detect if Download-C gets finished when we don't expect it to.

Comment 18

[Shawn Wilsher :sdwilsh]

So, the biggest problem here is that downloads might end up with the same id.  We check for more, so this approach is OK iff we use different source and targets so the function can catch the difference.

Comment 19

[(no longer active)]

Attachment: Don't use a timeout in the download manager test

(In reply to comment #18)
> So, the biggest problem here is that downloads might end up with the same id. 
> We check for more, so this approach is OK iff we use different source and
> targets so the function can catch the difference.

We already do this.  All three downloads have different source, destination and names.

I updated a couple of comments in the new version of the patch.  Other than that it's the same as the previous one.

Comment 20

[(no longer active)]

sdwilsh: ping?

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 349379 [details] [diff] [review]
Don't use a timeout in the download manager test

r=sdwilsh

Comment 22

[(no longer active)]

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/5a7748a40aa8>

Comment 23

[(no longer active)]

Comment on attachment 349379 [details] [diff] [review]
Don't use a timeout in the download manager test

This simplifies the download manager unit test for private browsing.  Nominating to get approval to land on 1.9.1.

Comment 24

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

Comment on attachment 349379 [details] [diff] [review]
Don't use a timeout in the download manager test

a191=beltzner

Comment 25

[:Gavin Sharp [email: [e-mail]]]

You don't need approval for tests! :)

Comment 26

[(no longer active)]

Attachment: mq patch for check-in on 1.9.1

Comment 27

[(no longer active)]

Landed on 1.9.1 <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/982c6ed92e70>

Comment 28

[(no longer active)]

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

Comment 29

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

test_privatebrowsing.js: PASS without the timer => Verified.