917012 - too many mainthread stat() calls during downloads

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[(dormant account)]

http://people.mozilla.org/~bgirard/cleopatra/#report=4ad168eda2697f1ada610ba03d4179799c903e1d

It's not right to call .exists during downloads. http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/allDownloadsViewOverlay.js#594

Comment 1

[Marco Castelluccio [:marco]]

Bug 827010?

Comment 2

[(dormant account)]

(In reply to Marco Castelluccio [:marco] from comment #1)
> Bug 827010?

There is no reason to do any syscalls here. We should have file size in gecko already.

Comment 3

[(dormant account)]

(In reply to Taras Glek (:taras) from comment #2)
> (In reply to Marco Castelluccio [:marco] from comment #1)
> > Bug 827010?
> 
> There is no reason to do any syscalls here. We should have file size in
> gecko already.

err...we should know that the file exists by nature of it being downloaded :)

Comment 4

[(dormant account)]

I should note that I had the downloads window open in background...but was interacting with main window and hitting these .exists janks

Comment 5

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

From an IRC discussion:
<gavin> seems to be a dupe of bug https://bugzilla.mozilla.org/show_bug.cgi?id=827010
<firebot> Bug 827010 nor, --, ---, nobody, NEW, In download views, use async I/O for retrieving data about the target and part files of downloads
<gavin> referenced in the code
<taras> gavin: right, except for the fact that we shouldnt stat() things we know the size of
<gavin> well it's not about knowing the size
<gavin> it's about the existence on disk
<gavin> we hide/disable the "Open file" menu option if you've since deleted the downloaded file, e.g.
<taras> gavin: sorry, same thing :)
<taras> we know a file exists if we are downloading it
<taras> :)
<gavin> this code shouldn't be called during a download
<gavin> well, I guess it could, if that results in selection changes
<taras> gavin: it's called periodically
<taras> without any interactions with download stuff
<gavin> that seems like the bug

Comment 6

[:Paolo Amadini]

This happens when the state of a download changes, if the Library window or the in-content Downloads view is open. Bug 827010 was filed to fix that, assuming the file exists right after completion, and using OS.File to check for its existence later.

Comment 7

[:Paolo Amadini]

Attachment: The patch

It turned out that bug 827010 was exacerbated by the fact that we sent
onStateChange notifications too often. This patch fixes the regression
introduced by the new Downloads API, while it leaves the general stat()
call issue to bug 827010. It might be that that bug will be addressed as
part of the refactoring to use the Downloads API without DataItem wrappers.

Comment 8

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/01de55e55a15

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01de55e55a15

Comment 10

[:Paolo Amadini]

Comment on attachment 809874 [details] [diff] [review]
The patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 847863
User impact if declined: Severe jank when Library window open during downloads
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Tested manually
String or IDL/UUID changes made by this patch: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d38bb5b38b7f