814700 - test_private_resume.js fails on Birch

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[(no longer active)]

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?

Comment 1

[Josh Matthews [:jdm]]

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?

Comment 2

[Andres Hernandez [:andreshm]]

 
> >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);

Comment 3

[(no longer active)]

Yeah, makes sense.

Comment 4

[(no longer active)]

(Now, this is the only failing xpcshell test on birch!)

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: 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

Comment 6

[(no longer active)]

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

Comment 7

[Andres Hernandez [:andreshm]]

(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!

Comment 8

[(no longer active)]

Comment on attachment 686782 [details] [diff] [review]
Patch v1

Cool!  Clearing the request for now, then!

Comment 9

[Andres Hernandez [:andreshm]]

Attachment: 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.

Comment 10

[(no longer active)]

Landed and screwed up the commit message meanwhile!

https://hg.mozilla.org/integration/mozilla-inbound/rev/31f4c1cb0e91

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31f4c1cb0e91