Closed Bug 451991 Opened 14 years ago Closed 14 years ago

nsIProgressEventSink::OnProgress reports wrong values when uploading


(Core :: Networking, defect)

Not set





(Reporter: smaug, Assigned: smaug)




(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
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?
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.