894586 - nsSyncLoadService::PushSyncStreamToListener should not fail for channels of unknown size

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mook (work)]

Attachment: Check for chunkSize < 1

See attached patch.

nsIChannel's documentation says that, when a channel doesn't know its content length, .contentLength may be -1.  nsSyncLoadService::PushSyncStreamToListener instead keeps it at -1, then casts it to uint32_t (in the call to NS_NewBufferedInputStream).  This causes the buffered input stream to attempt to (fallibly) allocate a 4GB buffer.  In my case this leads to an out of memory error, which means the thing fails to load.

The trivial patch attached checks for when chunkSize < 1 and sets it to the default (4k) instead.

I'm not familiar with the testing around this area - if it's necessary (which means constructing a channel of unknown length, somehow), please point to the nearby tests so I can copy things off them :)

(I'm picking reviewer based on previous commits, and component more-or-less randomly... Please redirect as necessary, thanks!)

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 776658 [details] [diff] [review]
Check for chunkSize < 1

This looks reasonable to me, but I'd love for someone in the networking team to sign off on this as well. jduell, can you either have a look or redirect as needed? Thanks!

Comment 2

[Jason Duell]

Comment on attachment 776658 [details] [diff] [review]
Check for chunkSize < 1

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

This looks fine to me.  So fine I might even be ok w/o a test.  But it would be nicer to have one :)

IIRC httpd.js adds a content-length header by default--I think that happens here:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#3964

In any case I'm fairly certain you can omit having a Content-Length in the response if you SeizePower!

  http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_duplicate_headers.js#61

(yes, I wrote that comment)

Comment 3

[Mook (work)]

Attachment: Now with a test

I can't help but think that this test is a horror in itself and we're better of not having it.  Seriously, WTF.

Comment 4

[Mook (work)]

Oh, I guess I should write more about that test:
- HTTP channels actually implement readSegments, so this code path isn't used at all.
- The only useful thing around I could find that *doesn't* implement readSegments was the file stream stuff, because it knows better
- Of course, file channels know how long they are, so... I implement a nsIChannel so I can say I don't :|
- The rest is all fluff needed to get to that point.

Incidentally, this matches better why we're seeing this bug... a JS-implemented channel that reads (eventually, after some other stuff) from a file stream.

Comment 5

[Jason Duell]

Comment on attachment 778195 [details] [diff] [review]
Now with a test

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

> I can't help but think that this test is a horror in itself 

You underestimate your very creative patch.  A work of art in its own, special way :)

Comment 6

[Mook (work)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f570fc641c5f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing xpcshell failures on all platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/009130768e09

https://tbpl.mozilla.org/php/getParsedLog.php?id=25802437&tree=Mozilla-Inbound

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Correction, Windows and Linux32 only.

Comment 9

[:Mook]

Attachment: fixed test

Sorry, difference between unpackaged and packaged tests :(
Fixed, now with a linux32 try run:

https://tbpl.mozilla.org/?tree=Try&rev=c500d6501af2

Comment 10

[Mook (work)]

Try 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc4bafd92a3
This time with a gree try run!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bc4bafd92a3