406857 - Don't clear referrer when restarting downloads

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

If you cancel a download, you'll see the referrer's TLD in the display, but when you restart it then finish/cancel, it becomes the download location.

Seems like we kill the referrer info when restarting.

Comment 1

[Ed Lee :Mardak]

Litmus would probably be start a download with a referrer like mozilla.com download firefox (make sure it's a mirror not from mozilla.com..), cancel the download, see it says mozilla.com; restart download and let download a bit; cancel; make sure it stil says mozilla.com; repeat once again for good justice.

Shawn: Any ideas for a testcase somehow setting up the channel to not report a referrer a second time through.. or however NS_GetReferrerFromChannel does its business..

Comment 2

[Ed Lee :Mardak]

Attachment: v1

Save the old referrer and reuse it if the new one is null.

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=5016

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 291521 [details] [diff] [review]
v1

r=sdwilsh

I would like a unit test on this before it lands.

Comment 5

[Shawn Wilsher :sdwilsh]

Any status change on this?

Comment 6

[Ed Lee :Mardak]

Any suggestions or you want to write the test? :) I suppose because we need to actually have a server response, we'll need to use the javascript http server. We could start a download in a canceled state with a referrer set and restart it.

Comment 7

[Ed Lee :Mardak]

Attachment: v1.1

Now with more testcase!

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304837 [details] [diff] [review]
v1.1

a=beltzner for 1.9

Comment 9

[Ed Lee :Mardak]

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.164; previous revision: 1.163
done
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js,v
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js,v  <--  browser_bug_406857.js
initial revision: 1.1
done

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out because the test failed on win2k3 and no-one was around.

Comment 11

[Ed Lee :Mardak]

Thanks roc.

Shawn: Is it okay if I just use settimeout 100? It's probably timing out because I send the enter key too early before the list populates. I thought it was going to be a problem, but I tried it without a timeout on my machine and it worked okay.

	FAIL - Timed out - chrome://mochikit/content/browser/toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js

Comment 12

[Shawn Wilsher :sdwilsh]

talk to sayrer?

Comment 13

[Ed Lee :Mardak]

Attachment: v1.2

Thought of something during my waltz class ;)

Comment 14

[Ed Lee :Mardak]

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.167; previous revision: 1.166
done
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js,v  <--  browser_bug_406857.js
new revision: 1.3; previous revision: 1.2
done

Comment 15

[Ed Lee :Mardak]

Relanding x3..

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.169; previous revision: 1.168
done
Checking in toolkit/mozapps/downloads/tests/browser/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js,v  <--  browser_bug_406857.js
new revision: 1.5; previous revision: 1.4
done

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022304 Minefield/3.0b4pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022308 Minefield/3.0b4pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008022308 Minefield/3.0b4pre

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Cotton-pickin' VM :-)

Last build ID should be:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008022304 Minefield/3.0b4pre

Comment 18

[Chris Cooper [:coop] (he/him)]

Any idea why the unit tests for this would be passing, but then timing out? 

I'm seeing this on two win2k3 boxes I'm trying to get setup right now to improve our unit test coverage.

Brief log at:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1207755300.1207755461.8150.gz

I've watched it run, and the Downloads window just sits there and doesn't close with referrer.goes as the only entry after the test completes.

Comment 19

[Ed Lee :Mardak]

Uh.. very strange. Right after the "ok()" for PASS - Got referrer on finish, it calls finish(), so I don't see why it would time out..

Comment 20

[Chris Cooper [:coop] (he/him)]

If I comment out the "file.remove(false);" immediately before the finish() in the unit test, the test finishes without timing out.

Thoughts on this? I'm not sure what file.remove(false) is doing, and I'll also note that this test works without change on other win2k3 boxes without the change.

Comment 21

[Ed Lee :Mardak]

Attachment: testcase update v1

Chris: Try this patch?

Comment 22

[Ed Lee :Mardak]

Oh duh. Windows has strange stuff with removing files, yeah try/catch it.

Comment 23

[Ed Lee :Mardak]

Attachment: testcase update v1.1

Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_406857.js,v  <--  browser_bug_406857.js
new revision: 1.7; previous revision: 1.6
done

Comment 24

[Chris Cooper [:coop] (he/him)]

That did the trick. Thanks!

Comment 25

[Shawn Wilsher :sdwilsh]

This unit test should have been an xpcshell unit test instead of a browser test.  Any reason why we went with a browser test instead?