Closed Bug 463021 Opened 11 years ago Closed 11 years ago

Private browsing tests use timeouts

Categories

(Firefox :: Private Browsing, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.1b3

People

(Reporter: mossop, Assigned: ehsan)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 2 obsolete files)

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.
I'll investigate this issue.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Depends on: 462986, 461710
Depends on: 463022
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.
Depends on: 464918
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.
Whiteboard: fixed when all deps get fixed
(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?
(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.
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.
(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?
(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
(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.
Well, does leaving private browsing mode close a download manager window if it's open?
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?
(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...
Then you should be dispatching the topic so the UI updates accordingly.
(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.
How does the UI currently know that it needs to update?
(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.
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.
Attachment #349149 - Flags: review?(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.
(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.
Attachment #349149 - Attachment is obsolete: true
Attachment #349379 - Flags: review?(sdwilsh)
Attachment #349149 - Flags: review?(sdwilsh)
sdwilsh: ping?
Attachment #349379 - Flags: review?(sdwilsh) → review+
Comment on attachment 349379 [details] [diff] [review]
Don't use a timeout in the download manager test

r=sdwilsh
Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/5a7748a40aa8>
Whiteboard: fixed when all deps get fixed
Target Milestone: --- → Firefox 3.2a1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #349379 - Flags: approval1.9.1?
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 on attachment 349379 [details] [diff] [review]
Don't use a timeout in the download manager test

a191=beltzner
Attachment #349379 - Flags: approval1.9.1? → approval1.9.1+
You don't need approval for tests! :)
Whiteboard: [baked on m-c; ready to land on 1.9.1]
Attached patch mq patch for check-in on 1.9.1 (obsolete) — Splinter Review
Landed on 1.9.1 <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/982c6ed92e70>
Keywords: fixed1.9.1
Whiteboard: [baked on m-c; ready to land on 1.9.1]
Attachment #353388 - Attachment is obsolete: true
Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.
Component: General → Private Browsing
QA Contact: general → private.browsing
Target Milestone: Firefox 3.2a1 → Firefox 3.1b3
test_privatebrowsing.js: PASS without the timer => Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.