test_private_resume.js fails on Birch

RESOLVED FIXED in mozilla20

Status

()

Toolkit
Downloads API
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: andreshm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla20
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=17298431&tree=Birch&full=1

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDownload.id]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/components/downloads/test/unit/head_download_manager.js :: addDownload :: line 122"  data: no]

Josh, is this a bug in the download manager patch?
(Reporter)

Updated

6 years ago
Blocks: 801653

Comment 1

6 years ago
Not a bug in the patch, just a change that is required in the implementation of adddDownload in head_download_manager.js. The id property of private downloads is not available when per-window is enabled, but the fix is harder than just obtaining the guid instead, since getDownloadByGUID is asynchronous:

>120   // This will throw if it isn't found, and that would mean test failure, so no
>121   // try catch block
>122   var test = dm.getDownload(dl.id);

I'm not certain what to do here. Maybe we can just eliminate this check. Marco?
(Assignee)

Comment 2

6 years ago
 
> >120   // This will throw if it isn't found, and that would mean test failure, so no
> >121   // try catch block
> >122   var test = dm.getDownload(dl.id);
> 
> I'm not certain what to do here. Maybe we can just eliminate this check.
> Marco?

You can add a check before, to do it only if not private. 

> if (!aParams.isPrivate)
>   var test = dm.getDownload(dl.id);
(Reporter)

Comment 3

6 years ago
Yeah, makes sense.
(Reporter)

Comment 4

6 years ago
(Now, this is the only failing xpcshell test on birch!)
(Assignee)

Updated

6 years ago
Assignee: nobody → andres
(Assignee)

Comment 5

6 years ago
Created attachment 686782 [details] [diff] [review]
Patch v1

I migrated the test, however with the current per-window PB mode the private downloads can't be resume, paused or cancel, so this test checks only that. 

There is already a test that checks the resume on a public window:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_resume.js
Attachment #686782 - Flags: review?(ehsan)
(Reporter)

Comment 6

6 years ago
Why do you say that we cannot pause/resume/cancel private downloads in per-window mode?  That should definitely not be happening!
(Assignee)

Comment 7

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Why do you say that we cannot pause/resume/cancel private downloads in
> per-window mode?  That should definitely not be happening!

My mistake, I checked that I was using Services.downloads.cancelDownload(download.id) and it's not possible to get the id from a private download: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#3262, so that fails. And also that I was trying to access cancel() and pause() before the download begins, so it fails too.  

I'll update the patch! Thanks!
(Reporter)

Comment 8

6 years ago
Comment on attachment 686782 [details] [diff] [review]
Patch v1

Cool!  Clearing the request for now, then!
Attachment #686782 - Flags: review?(ehsan)
(Assignee)

Comment 9

6 years ago
Created attachment 687142 [details] [diff] [review]
Patch v2

Fixed the patch, now it test pause and resume correctly. 
Also, I updated the head and related tests to use a common method.
Attachment #686782 - Attachment is obsolete: true
Attachment #687142 - Flags: review?(ehsan)
(Reporter)

Updated

6 years ago
Attachment #687142 - Flags: review?(ehsan) → review+
(Reporter)

Comment 10

6 years ago
Landed and screwed up the commit message meanwhile!

https://hg.mozilla.org/integration/mozilla-inbound/rev/31f4c1cb0e91
https://hg.mozilla.org/mozilla-central/rev/31f4c1cb0e91
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.