Closed Bug 1159632 Opened 10 years ago Closed 9 years ago

TEST-UNEXPECTED-FAIL | toolkit/components/jsdownloads/test/unit/test_DownloadImport.js | xpcshell return code: 0

Categories

(Toolkit :: Downloads API, defect)

Product:
Toolkit
Component:
Downloads API
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- fixed
thunderbird38 - fixed
thunderbird41 --- fixed

People

(Reporter: Fallen, Assigned: mkmelin)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

bug1159632_v3.patch
1.59 KB, patch
Paolo
: review+
Details | Diff | Splinter Review
Seeing this on beta :-(
Based on the timing of the landing, and the location of the bug, I believe this is a regression from bug 1009465. Magnus was the promoter of this landing on mozilla 38, perhaps you could sort out the test failure?
Blocks: 1009465
Flags: needinfo?(mkmelin+mozilla)
Version: 30 → 38
I wonder if we're using the branch/version we should now... See bug 1022816 comment 54 - 57. I'll have to check if that's included in what we're building. 10:41:15 INFO - NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove] 10:41:15 INFO - getTempFile/<@C:/slave/test/build/tests/xpcshell/tests/toolkit/components/jsdownloads/test/unit/head.js:139:6 10:41:15 INFO - _execute_test@C:\slave\test\build\tests\xpcshell\head.js:557:15 10:41:15 INFO - @-e:1:0
That code appears to be landed where it should.
Magnus, could you please sort this out? Is it a regression we should be concerned about?
We have not had any interest in the bug in over two weeks. Looking at the test, it has to do with importing previous versions of the download history. Given that saving of downloads was mostly broken in Thunderbird 31 anyway, even if feature is broken I don't think we should block on it. So I am removing blocking for tb38, though I would prefer if we tried to sort it out.
tracking-thunderbird38: + → -
tracking-thunderbird_esr38: --- → +
Ok so I investigated this a bit more. Looks partly like the same mystery as in bug 1072748 comment 10. I don't know exactly why there's the ERROR NS_ERROR_FILE_ACCESS_DENIED, but the test isn't failing on mozilla-esr38 because browser.helperApps.deleteTempFileOnExit is false there while running. browser.helperApps.deleteTempFileOnExit is true in thunderbird, and when running the tests for thunderbird. For firefox it is also true, BUT false while running the xpcshell tests! I just verified it fails for firefox too on mozilla-esr38 if I set the pref to true in all.js directly. Paolo: any ideas on the cause, or what might have fixed it for everything > 38?
Flags: needinfo?(mkmelin+mozilla) → needinfo?(paolo.mozmail)
Thanks for looking into this, it was quite helpful! (In reply to Magnus Melin from comment #39) > I don't know exactly why there's the ERROR NS_ERROR_FILE_ACCESS_DENIED > the test isn't failing on mozilla-esr38 because > browser.helperApps.deleteTempFileOnExit is false there while running. Windows delays the actual deletion of files, so "exists" may still return true for a deleted file for a short time. We handle that in production code, but not in the "trivial" remove case in test code. Apparently this causes a race condition with the delete on shutdown. The fix could be to change the test code to use OS.File.remove with a catch for both access denied and file doesn't exist errors, similar to what we have in other production code: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadStore.jsm#188 >, or what might have fixed it for everything > 38? No clue here... seems to be dependent on timing though, so it may be just chance.
Flags: needinfo?(paolo.mozmail)
Since we see this on comm-central trunk now also (randomly) you're probably right that it's just timing.
Attached patch bug1159632.patch (obsolete) — — Splinter Review
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8610200 - Flags: review?(paolo.mozmail)
Filed bug 1168178 for xpcshell-test not applying the firefox prefs.
Comment on attachment 8610200 [details] [diff] [review] bug1159632.patch Sorry for the delay, I just received the bugmail for this bug. I suspect the "exists()" call may cause an access denied error as well, and the "remove()" call may generate a file not exists error if it races with the other deletion, so it may be better to always call "remove()" and catch the file not exists error as well.
Attachment #8610200 - Flags: review?(paolo.mozmail)
Attached patch bug1159632.patch (obsolete) — — Splinter Review
Updated per review suggestion.
Attachment #8610200 - Attachment is obsolete: true
Attachment #8613681 - Flags: review?(paolo.mozmail)
Comment on attachment 8613681 [details] [diff] [review] bug1159632.patch Review of attachment 8613681 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay, recently Bugzilla notifications are behaving weirdly, and this review slipped through the cracks. Looks good to me. Nit: technically the "instanceof" check should be the first one, since it implicitly calls QueryInterface if needed and prevents further errors if the exception is not an object. (Makes little difference for our purposes here though.)
Attachment #8613681 - Flags: review?(paolo.mozmail) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-thunderbird38: --- → affected
status-thunderbird41: --- → fixed
Component: General → Download Manager
Product: Thunderbird → Toolkit
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Version: 38 → unspecified
Bugs are not resolved before they reach mozilla-central. Also, could we please stop adding new instances of the non-standard `catch (e if foo)` syntax?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to :Ms2ger from comment #66) > Bugs are not resolved before they reach mozilla-central. Also, could we > please stop adding new instances of the non-standard `catch (e if foo)` > syntax? So what is the standard syntax equivalent?
Flags: needinfo?(mkmelin+mozilla) → needinfo?(Ms2ger)
Just plain catch (e) { if (foo) { ... } else { throw e; } } is the best alternative, sadly.
Flags: needinfo?(Ms2ger)
Windows uses NS_ERROR_FILE_NOT_FOUND, not NS_ERROR_FILE_TARGET_DOES_NOT_EXIST... https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e999a8e685a
Attachment #8613681 - Attachment is obsolete: true
Comment on attachment 8617564 [details] [diff] [review] bug1159632_v3.patch Review of attachment 8617564 [details] [diff] [review]: ----------------------------------------------------------------- Updated for Cr.NS_ERROR_FILE_NOT_FOUND and to use throw instead. Try's all green
Attachment #8617564 - Flags: review?(paolo.mozmail)
Comment on attachment 8617564 [details] [diff] [review] bug1159632_v3.patch (In reply to Magnus Melin from comment #76) > Windows uses NS_ERROR_FILE_NOT_FOUND, not > NS_ERROR_FILE_TARGET_DOES_NOT_EXIST... Oh, wow. I'm glad we're moving away from this towards saner OS.File errors :-)
Attachment #8617564 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/4674ad6dba74
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: