Closed Bug 894586 Opened 10 years ago Closed 10 years ago

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


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

Not set





(Reporter: mark.yen, Assigned: mark.yen)



(1 file, 2 obsolete files)

Attached patch Check for chunkSize < 1 (obsolete) — Splinter Review
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!)
Attachment #776658 - Flags: review?(ehsan)
Attachment #776658 - Flags: review?(ehsan) → review?(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!
Attachment #776658 - Flags: review?(jst)
Attachment #776658 - Flags: review?(jduell.mcbugs)
Attachment #776658 - Flags: review+
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:

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

(yes, I wrote that comment)
Attachment #776658 - Flags: review?(jduell.mcbugs) → review+
Attached patch Now with a test (obsolete) — Splinter Review
I can't help but think that this test is a horror in itself and we're better of not having it.  Seriously, WTF.
Attachment #776658 - Attachment is obsolete: true
Attachment #778195 - Flags: review?(jduell.mcbugs)
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 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 :)
Attachment #778195 - Flags: review?(jduell.mcbugs) → review+
Correction, Windows and Linux32 only.
Attached patch fixed testSplinter Review
Sorry, difference between unpackaged and packaged tests :(
Fixed, now with a linux32 try run:
Attachment #778195 - Attachment is obsolete: true
Attachment #782593 - Flags: review?(jduell.mcbugs)
Attachment #782593 - Flags: review?(jduell.mcbugs) → review+
Try 2:
This time with a gree try run!
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.