376660 - [FIX]Unichar stream loader should provide a channel even if there is no data

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Right now, lack of data in the loader means GetChannel() returns null in OnStreamComplete.  That doesn't really match the API comments (and bit me, in fact).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

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

Comment on attachment 260767 [details] [diff] [review]
Like so

makes sense

Comment 3

[Boris Zbarsky [:bzbarsky]]

Fixed.  Should write a unit test...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: How's this for the test?

Comment 5

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

Comment on attachment 261624 [details] [diff] [review]
How's this for the test?

hm. isn't it a bug that the data is null here? per the API comments it should be null in failure cases, and I don't think an empty string as data is a failure condition.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm....  Allocating a stream for no data seems silly, though.  Maybe we should change the comments.  ;)

Comment 7

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

Comment on attachment 261624 [details] [diff] [review]
How's this for the test?

OK. please fix the comment or at least file a bug to do so and r=biesi

(f seems like an odd name for an nsIUnicharStreamLoader though)

a test that actually tests a failure case would be nice too, though (perhaps an http channel for localhost on port 0? or even an invalid port (>65535))

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in test with comments.

Comment 9

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

Attachment: test a failure case too

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 261867 [details] [diff] [review]
test a failure case too

>+      do_test_finished();

Why not call that in done() and avoid eating the exceptions?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 261867 [details] [diff] [review]
test a failure case too

r- pending answer to my question

Comment 12

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

Hm...

I could, and call do_test_pending just once. But how would that affect whether the exception is eaten?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Then you wouldn't need the try {} around the tests[current_test++]() call, no?

Comment 14

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

the try..finally is so that I call do_test_finished even if the do_check_* fail. this is necessary because otherwise the test would hang. (the exception is still thrown)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, that's a finally, not a catch.  Nevermind!

Comment 16

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

checked in.

Checking in test_bug376660.js;
/cvsroot/mozilla/netwerk/test/unit/test_bug376660.js,v  <--  test_bug376660.js
new revision: 1.2; previous revision: 1.1
done