Closed Bug 451991 Opened 11 years ago Closed 11 years ago

nsIProgressEventSink::OnProgress reports wrong values when uploading

Categories

(Core :: Networking, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(3 files, 1 obsolete file)

Occasionally when creating lots of XMLHttpRequest objects, so that some of them
have to wait others to finish, OnProgress seems to get called with
parameters where aProgress > aProgressMax.
aProgressMax does have the right value (size of headers + size of content), so
it is aProgress which is wrong.
I used this http://mozilla.pettay.fi/xhr_upload/xhr_upload_demo_test.html
to test. It dumps info to console if something goes wrong. Just create 5 XHRs, and
press the button few times.
I verified the problem with some printfs in XHR ::OnProgress.
It seems that nsSocketOutputStream::Write increases mByteCount too much.
mByteCount gets increased up to twice too much?
I'm very unfamiliar with necko code, still trying to find why that happens
and if mByteCount should be reseted somwhere or if nsSocketOutputStream::Write is 
called too often...
XHR uploads the right data, so aProgress doesn't reflect the actually amount
of sent data.
nsSocketOutputStream::Write increases mByteCount by the number it returns for bytes written: the return value of PR_Write.  That seems correct, no?
Or perhaps some extra data is sent, but content-length has still the original value. /me continues debugging.
Finally found the problem, sort for. It is in nsHttpConnection::Activate, which 
doesn't recreate SocketTransport if there is one already. And that causes
mSocketOut to be reused, but without re-initializing it in any way.
Will try to come up with a patch (first ever in necko).
Assignee: nobody → Olli.Pettay
And for download aProgress is right because it is synthesized in nsHttpChannel::OnDataAvailable
Attached patch Clear counters (obsolete) — Splinter Review
So far I haven't found any better option, but as I said, I'm not familiar with this code.
Attachment #335514 - Flags: review?(cbiesinger)
Blocks: 435425
I don't really like messing with the socket transport like that. Can you handle this in the HTTP code instead?
Attachment #335514 - Attachment is obsolete: true
Attachment #335886 - Flags: superreview?
Attachment #335886 - Flags: review?(cbiesinger)
Attachment #335514 - Flags: review?(cbiesinger)
Attachment #335886 - Flags: superreview? → superreview?(cbiesinger)
Attachment #335886 - Flags: superreview?(cbiesinger)
Attachment #335886 - Flags: superreview+
Attachment #335886 - Flags: review?(cbiesinger)
Attachment #335886 - Flags: review+
Comment on attachment 335886 [details] [diff] [review]
update progress value in http code

+        nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mRequestStream);
+        if (!seekable) {

I don't see how this QI can fail. Can you replace the if with an assertion?
Attached patch with assertionSplinter Review
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.