561063 - Some unit (xpcshell) tests freeze when localhost can't be resolved

Status: NEW

(Core :: General, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Partial patch

While machines where localhost can't be resolved are broken in an interesting way, it would still be best if the tests wouldn't freeze waiting for pending tests and just fail instead.

So far, I only have a list of tests from the 1.9.1 test suite:
test_dm/unit/test_bug_395092.js
test_dm/unit/test_resume.js
test_extensionmanager/unit/test_bug430120.js
test_extensionmanager/unit/test_bug449027.js
test_extensionmanager/unit/test_bug455906.js
test_necko/unit/test_bug331825.js
test_necko/unit/test_bug369787.js
test_necko/test/test_start_stop.js

I could find a fix for the download manager and necko tests, but I can't find a way to avoid it in the blocklist tests (which the 3 extensionmanager tests really are).

Comment 1

[Mike Hommey [:glandium]]

From a forced failure run with current trunk, I also get errors (assuming tests don't normally take more than a minute individually) with:
test_dm/unit/test_history_expiration.js
test_dm/unit/test_lastmodified_time_preservation.js
test_extensionmanager/unit/test_bug514327_3.js
test_necko/unit/test_multipart_streamconv.js

Comment 2

[Mike Hommey [:glandium]]

test_dm/unit/test_history_expiration.js doesn't freeze, it only takes more than 1 minute.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Patch for all freezing tests

This time, with workarounds for all the freezing tests on m-c, including the em tests, with a xmlhttprequest wrapper that looks ugly to me so if you have a better idea, I'm all ears.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

don't do if (status); do if (!Components.isSuccessCode(status))

Comment 5

[Daniel Holbert [:dholbert]]

Reporting what looks like an instance of this bug:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1291183390.1291190768.23014.gz
WINNT 5.2 mozilla-central debug test xpcshell on 2010/11/30 22:03:10
s: w32-ix-slave41

TEST-INFO | e:\builds\moz2_slave\mozilla-central-win32-debug-unittest-xpcshell\build\xpcshell\tests\toolkit\mozapps\extensions\test\xpcshell-unpack\test_bug449027.js | running test ...

command timed out: 7200 seconds elapsed

Comment 6

[Mike Hommey [:glandium]]

I don't think it is, but the root of the problem is the same: xpcshell tests never time out by themselves, and if something goes wrong, they must be killed by hand.

Comment 7

[Robert Sayre]

Jeff will have a better idea of what to do with this, or who should review it.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 441064 [details] [diff] [review]
Patch for all freezing tests

Review of attachment 441064 [details] [diff] [review]:
-----------------------------------------------------------------

So, um.  I thought if asyncOpen succeeded, then the observer passed in was supposed to have both its onStartRequest and onStopRequest called.  If that's the case, as I understand nsIChannel to supposedly guarantee, then why are these changes necessary?

That said.  I'm leaving for vacation tomorrow, so I am not the best person to be asked to review this right now, if I can't immediately answer that question, because otherwise you're not going to see any response on this until September.  So I'm passing this off, and hopefully you'll get a faster response (and a more intelligent one) from biesi than you would if you waited on me.  (And if the nick's any indication, it might be worth pinging biesi on IRC rather than expecting bugmail will get a response, but I don't know.)

::: netwerk/test/httpserver/test/test_start_stop.js
@@ +163,5 @@
>  
> +  var onStartRequest = function(request, context) {
> +    if (request.status)
> +      throw("Unexpected error " + request.status);
> +  };

Perhaps this should be changed in head_utils.js instead, if the problem is a generic one?  I'm not sure I understand this.  If asyncOpen, called underneath runHttpTests, doesn't throw, shouldn't it definitely call both onStartRequest and onStopRequest?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Waldo, the problem is basically that if the connection fails, onStartRequest and onStopRequest will indeed get called but accessing various attributes of the channel will throw an exception. That means that do_test_finished will never get called, so the event loop never exits. Calling do_throw also sets a flag to terminate the event loop, which is why this works.

You can't necessarily do this change in a generic way as Waldo suggested because some tests might test the failure cases. So you still want to execute onStartRequest in that case.

That said: This patch is more than a year old, I'm _sure_ it needs updating. Please also fix the issue from comment 4. And since the above is not intuitive, please do add more comments. I agree with Waldo that a generic approach to fix more tests would be nice although I'm not sure what that could look like.

Comment 10

[Mike Hommey [:glandium]]

Looked at the test corresponding to the first part of the patch, and it seems to still be true. However, I'm clearly not working on this.