553995 - test_bug_406857.js blocks when example.com is not reachable

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Patch

When example.com is not reachable for any reason (firewall, lack of network connectivity, ...), test_bug_406857.js blocks indefinitely.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I was going to suggest using mochi.test, but it looks like that change doesn't apply to xpcshell tests (I guess because they don't use the same PAC config as mochitest-based tests?).

Comment 2

[Mike Hommey [:glandium]]

AFAICS, xpcshell tests don't use a PAC config at all.

Comment 3

[Shawn Wilsher :sdwilsh]

This doesn't seem to apply cleanly, and only has three lines of context.  Can you upload an updated patch with more context please?

Comment 4

[Mike Hommey [:glandium]]

(In reply to comment #3)
> This doesn't seem to apply cleanly, and only has three lines of context.  Can
> you upload an updated patch with more context please?

I don't know where you tried to apply the patch but it applies cleanly here on m-c, without any fuzz or offset.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 433899 [details] [diff] [review]
Patch

constify the port number and and use it in both places.

Also, make sure you stop the http server please.

In the future please upload patches with more context.

r=sdwilsh with those changes.

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> (From update of attachment 433899 [details] [diff] [review])
> constify the port number and and use it in both places.
> 
> Also, make sure you stop the http server please.

Interestingly, none of the above is being done in other tests using the http server. The former, apparently nowhere, and the latter apparently only on a few download manager tests. I do wonder if that wouldn't be a bug ? (for the latter, especially considering that I sometimes have xpcshell tests freezing)

Comment 7

[Mike Hommey [:glandium]]

Actually, head_download_manager takes care of stopping the http server if a global httpserv variable is used for it. As for my xpcshell tests freeze, it's because of unhandled failures in the test suite... that leads to do_test_pending without the corresponding do_test_finished. I'll file a new bug about that.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Interestingly, none of the above is being done in other tests using the http
> server. The former, apparently nowhere, and the latter apparently only on a few
> download manager tests. I do wonder if that wouldn't be a bug ? (for the
> latter, especially considering that I sometimes have xpcshell tests freezing)
Yeah, shame on me for not pushing for having the port number constified in other download manager tests.  I've been trying to push to get things clearer as new tests are written or old ones revised to improve this situation.

As for stopping the http server, I bet it can manage itself, but again, I'd prefer to be explicit in tests.

Comment 9

[Mike Hommey [:glandium]]

(In reply to comment #8)
> As for stopping the http server, I bet it can manage itself, but again, I'd
> prefer to be explicit in tests.

Well, in download manager tests, it appears head_download_manager already handles this, so doing it explicitely will actually fail, since the httpserver throws in case it is already stopped. Sure, we could put the stop in a catch clause but that sounds so wrong.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> Well, in download manager tests, it appears head_download_manager already
> handles this, so doing it explicitely will actually fail, since the httpserver
> throws in case it is already stopped. Sure, we could put the stop in a catch
> clause but that sounds so wrong.
But only the global httpserv (which this test does not use), and only if you call getDownloadListener, which this test does not use.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Patch v2

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 440719 [details] [diff] [review]
Patch v2

r=sdwilsh

Thanks for putting up with my pickiyness.

Comment 13

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/2d1b5789e211