Closed Bug 1061112 Opened 6 years ago Closed 6 years ago

test_app_rep.js fails when "browser.safebrowsing.downloads.enabled" is false or not set

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch The fixSplinter Review
Not quite sure when this was introduced, however, it is currently failing on Thunderbird:

17:49:30  WARNING -  TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/downloads/test/unit/test_app_rep.js | 2147549183 == 2147746065 - See following stack:
17:49:30     INFO -  /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/downloads/test/unit/test_app_rep.js:onComplete:145
17:49:30     INFO -  /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/downloads/test/unit/test_app_rep.js:test_nullSourceURI:141

2147746065 is NS_ERROR_NOT_AVAILABLE, and digging into the code reveals that the "browser.safebrowsing.downloads.enabled" not having a value in Thunderbird, causes it to return this value.

Setting the pref in the test fixes the issue.
Attachment #8482157 - Flags: review?(paolo.mozmail)
Duplicate of this bug: 1061119
Depends on: 1057848
Comment on attachment 8482157 [details] [diff] [review]
The fix

Seems bug 1057848 is related, but didn't look in detail. Redirecting to mmc.
Attachment #8482157 - Attachment is patch: true
Attachment #8482157 - Flags: review?(paolo.mozmail) → review?(mmc)
Comment on attachment 8482157 [details] [diff] [review]
The fix

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

Given that Thunderbird doesn't have this pref set, I assume it can't use the functionality yet?
Attachment #8482157 - Flags: review?(mmc) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> Given that Thunderbird doesn't have this pref set, I assume it can't use the
> functionality yet?

We haven't looked at what is needed for porting it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bed5e74a36b9
Assignee: nobody → standard8
Target Milestone: --- → mozilla35
Comment on attachment 8482157 [details] [diff] [review]
The fix

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

You may want to apply the same fix to test_app_rep_windows.js in the same directory.
Keywords: leave-open
Attached patch Fix for windows (obsolete) — Splinter Review
For windows, "urlclassifier.download[Block|Allow]Table" are also needed.

try server results:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e0c42357b44c
Attachment #8486225 - Flags: review?(mmc)
Comment on attachment 8486225 [details] [diff] [review]
Fix for windows

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

Thanks, Hiro! Just a couple minor fixes.

::: toolkit/components/downloads/test/unit/test_app_rep_windows.js
@@ +192,5 @@
> +  Services.prefs.setCharPref("urlclassifier.downloadAllowTable",
> +                             "goog-downloadwhite-digest256");
> +  do_register_cleanup(function() {
> +    Services.prefs.clearUserPref("urlclassifier.downloadBlockTable");
> +    Services.prefs.clearUserPref("urlclassifier.downloadAllowTable");

There should only be one do_register_cleanup. Can you move these above to the existing function? Similarly, can you move all of the pref setting before the test starts together?
Attachment #8486225 - Flags: review?(mmc)
Moved those lines.
Attachment #8486225 - Attachment is obsolete: true
Attachment #8486808 - Flags: review?(mmc)
Attachment #8486808 - Flags: review?(mmc) → review+
https://hg.mozilla.org/mozilla-central/rev/50e9d1718ee7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.