Closed Bug 1057764 Opened 10 years ago Closed 10 years ago

Download Manager regression in FF32 beta 9 update

Categories

(Firefox :: Downloads Panel, defect)

Product:
Firefox
Component:
Downloads Panel
Version:
32 Branch
Platform:
All
Windows 7
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 34
Tracking Flags:
Tracking Status
firefox31 --- unaffected
firefox32 + verified
firefox33 --- verified
firefox34 --- verified

People

(Reporter: philipp, Assigned: mmc)

References

Details

(Keywords: regression)

Attachments

(2 files)

download stuck
11.52 KB, image/png
Details
Remove references to appRepUrl completely (
2.78 KB, patch
gcp
: review+
lmandel
: approval-mozilla-aurora+
lmandel
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached image download stuck — 
this is a major regression in the latest update from ff32 beta 8 to beta 9 and was reported in sumo threads and on irc:
https://support.mozilla.org/questions/1016710
https://support.mozilla.org/questions/1016723

STR in a clean profile:
- download & save any file, for example: http://download.documentfoundation.org/libreoffice/stable/4.3.0/win/x86/LibreOffice_4.3.0_Win_x86.msi.torrent
- download manager will be stuck on "unknown time remaining"
- when you try to clear the download this won't work, its status will change to "paused"
this is apparently related to safe-browsing. when compared to beta 8, this string is missing in about:config:
 browser.safebrowsing.appRepURL;https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%

once manually added, the problem goes away...
Component: Untriaged → Downloads Panel
It's fixed in FF33+.
Blocks: 1055670
[Tracking Requested - why for this release]:
status-firefox31: --- → unaffected
status-firefox33: --- → unaffected
status-firefox34: --- → unaffected
tracking-firefox32: --- → ?
Keywords: regression
Matt, are you able to reproduce? Seems strange according go https://bugzilla.mozilla.org/show_bug.cgi?id=1055670#c32
Assignee: nobody → mmc
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8477912 - Flags: review?(gpascutto)
Flags: needinfo?(mwobensmith)
So the bug is that if the pref is gone, then this:
NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl), NS_ERROR_NOT_AVAILABLE);
will fail to call OnComplete(false, NS_ERROR_NOT_AVAILABLE), right?

https://bugzilla.mozilla.org/show_bug.cgi?id=1055670#c32 Note the nuance "pref is blank" vs "pref does not appear" Ouch! Doesn't explain why it appeared to work, though.
(In reply to Loic from comment #2)
> It's fixed in FF33+.

That's because it's Aurora, the disabling from bug 1055670 only applies to Beta and Release branches.
Comment on attachment 8477912 [details] [diff] [review]
Remove references to appRepUrl completely (

Review of attachment 8477912 [details] [diff] [review]:
-----------------------------------------------------------------

I hope nothing else depends on that pref actually being there :-/
Attachment #8477912 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #9)
> So the bug is that if the pref is gone, then this:
> NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl),
> NS_ERROR_NOT_AVAILABLE);
> will fail to call OnComplete(false, NS_ERROR_NOT_AVAILABLE), right?

Monica, you should file a follow-up bug for this BTW.
I can confirm that adding "browser.safebrowsing.appRepURL;https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%" to about:config immediately fixes the problem.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #9)
> So the bug is that if the pref is gone, then this:
> NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl),
> NS_ERROR_NOT_AVAILABLE);
> will fail to call OnComplete(false, NS_ERROR_NOT_AVAILABLE), right?

Yes. I think this is the safest option that we have for fixing at this point, so I'm going to go ahead and check in then nominate for uplift.

If anyone who is affected by this bug could try this build *without* modifying any preferences, I would appreciate it: https://tbpl.mozilla.org/?tree=Try&rev=6474ccc9d73c
Comment on attachment 8477912 [details] [diff] [review]
Remove references to appRepUrl completely (

Approval Request Comment
[Feature/regressing bug #]: https://bugzilla.mozilla.org/show_bug.cgi?id=1055670

[User impact if declined]: Downloads stuck on Windows

[Describe test coverage new/current, TBPL]: Please wait for someone to manually verify TBPL push on windows

[Risks and why]: High risk since it's late in cycle. Alternative is worse, though.

[String/UUID change made/needed]: none.
Attachment #8477912 - Flags: approval-mozilla-beta?
Attachment #8477912 - Flags: approval-mozilla-aurora?
(In reply to Gian-Carlo Pascutto [:gcp] from comment #12)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #9)
> > So the bug is that if the pref is gone, then this:
> > NS_ENSURE_SUCCESS(Preferences::GetCString(PREF_SB_APP_REP_URL, &serviceUrl),
> > NS_ERROR_NOT_AVAILABLE);
> > will fail to call OnComplete(false, NS_ERROR_NOT_AVAILABLE), right?
> 
> Monica, you should file a follow-up bug for this BTW.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1057848
> If anyone who is affected by this bug could try this build *without*
> modifying any preferences, I would appreciate it:
> https://tbpl.mozilla.org/?tree=Try&rev=6474ccc9d73c

Correction: please set browser.safebrowsing.appRepURL to empty and try a windows build from

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mchew@mozilla.com-6474ccc9d73c/try-win32/
thank you, when i tested your try build with a clean profile, downloads finished without a problem out of the box and when browser.safebrowsing.appRepURL is manually set to an empty string as well. i hope others can confirm that...
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #18)
> > If anyone who is affected by this bug could try this build *without*
> > modifying any preferences, I would appreciate it:
> > https://tbpl.mozilla.org/?tree=Try&rev=6474ccc9d73c
> 
> Correction: please set browser.safebrowsing.appRepURL to empty and try a
> windows build from
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mchew@mozilla.com-
> 6474ccc9d73c/try-win32/

I tried.
In both cases, browser.safebrowsing.appRepURL empty or with its default string chain, the download finishes.
OS: Windows 7 → All
Hardware: x86_64 → All
The regression is Windows specific though admittedly we don't have generic "Windows" platform in Bugzilla.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)
> The regression is Windows specific though admittedly we don't have generic
> "Windows" platform in Bugzilla.

Sorry, didn't wanted to change the platform but only the bits. Reverted now.
OS: All → Windows 7
https://hg.mozilla.org/mozilla-central/rev/8be3a7df7779
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify+
Flags: in-testsuite?
Flags: in-qa-testsuite?(hskupin)
Comment on attachment 8477912 [details] [diff] [review]
Remove references to appRepUrl completely (

mmc confirmed that this has been tested and that there are no additional places where the pref that caused the issue is referenced. Approved for aurora and beta.
Attachment #8477912 - Flags: approval-mozilla-beta?
Attachment #8477912 - Flags: approval-mozilla-beta+
Attachment #8477912 - Flags: approval-mozilla-aurora?
Attachment #8477912 - Flags: approval-mozilla-aurora+
Thanks lmandel, I'm in meetings for the next hour so if someone wants to check in before then, please go ahead.
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #6)
> Matt, are you able to reproduce? Seems strange according go
> https://bugzilla.mozilla.org/show_bug.cgi?id=1055670#c32


Talking to Monica, my mistake was that I tested the download with EXEs that were already signed, which skip the application reputation check. I re-tested with an unsigned EXE and I can reproduce this problem. I will verify the fix when it's checked in.
Flags: needinfo?(mwobensmith)
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1df4eee7044
https://hg.mozilla.org/releases/mozilla-beta/rev/92ad4cfa9435

Note: I am *not* sure if what TBPL produces is the same as the actual release build, due to things like configuring the google safebrowsing api key. Nevertheless, it would be great if Matt or Juan could test on Windows when those builds are complete.
(In reply to Loic from comment #2)
> It's fixed in FF33+.

Now it's getting stuck whenever i try to open a torrent file. i have tried various torrent websites and it always give a message like: ex 15.5 kb opened of 15.3 kb.. unknown time remaining. like it's opened more kb's than the actual size of the torrent file itself. again, i have tried many different torrent websites.. i can download pictures without problem, but whenever i try to download a torrent now.. it gets stuck.. it didn't do this last week.. firefox had an update last week and then another 1 the following day after and now i can't open torrent files anymore... HELP!
Flags: needinfo?(mmc)
Kurt, what version of Firefox are you on? This bug is specific to Firefox 32 Beta.
Flags: needinfo?(mmc)
Kurt, this bug was introduced in FF 32 beta 9. It will be fixed in FF 32 rc1, which is going to build today.
Monica, can we get a mochitest for this particularly major regression? I would be more confident in seeing this test run by buildbot instead of an external framework.
Flags: in-qa-testsuite?(hskupin) → needinfo?(mmc)
Henrik, there was actually an xpcshell test for this: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8387e2ab335

I need to figure out why the callback wasn't reached in beta. Also, because so many of the safebrowsing checks depend on the presence of lists and responses from remote servers, the amount of testing we can do in mochitest and xpcshell is limited. That's why tests like https://bugzilla.mozilla.org/show_bug.cgi?id=1018161 are so important.
Flags: needinfo?(mmc)
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #42)
> Henrik, there was actually an xpcshell test for this:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c8387e2ab335
> 
> I need to figure out why the callback wasn't reached in beta. 

The test clears the pref, but does not remove it. As per comment 9, removing will cause the NS_ENSURE_SUCCESS to return without calling the callback. (IIRC we disallowed this construct in the C++ coding guidelines for new code recently, exactly because it produces errors like this)

You could add a test with clearing vs resetting the pref, but I actually agree with Monica that this really has to be tested in a setup with network access. The buildbot tests mess with the exact same pref that caused the error because they don't want to access the network. This means that they're never really testing what the real build actually does.
My second paragraph should've read "clearing vs removing".
(In reply to Gian-Carlo Pascutto [:gcp] from comment #44)
> agree with Monica that this really has to be tested in a setup with network
> access. The buildbot tests mess with the exact same pref that caused the
> error because they don't want to access the network. This means that they're
> never really testing what the real build actually does.

Sounds good then. Would you mind to create a new bug in Mozilla QA / Mozmill Tests, which describes exactly what you want to have? You know better than me all the details to check. We can then work on a test with higher priority. Thanks.
Filed Bug 1058420 for more MozMill coverage.
Thanks, we will start working on a test soon.
I've verified in today's Fx32 RC, Fx33 and Fx34 that downloads of both signed and unsigned EXEs as well as other file types download correctly. The pref is no longer present in Fx32 as well. 

I'm marking verified. If there are other tests that need to be performed, please let me know and I'll do those ASAP. Thank you.
Status: RESOLVED → VERIFIED
status-firefox32: fixed → verified
status-firefox33: fixed → verified
status-firefox34: fixed → verified
Hi Matt.

The latest fix worked for me (FF 32 beta, windows 7 64-bit).
Yup! I no longer have this issue, thanks Mozilla team!
(In reply to james.combs7 from comment #64)
> Yup! I no longer have this issue, thanks Mozilla team!

You're welcome, and I apologize for introducing the issue in the first place.
The issue is gone for me as well after 20140825202822. Thanks!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: