The asserting code is changeset: 72136:eb5866601f88 user: Honza Bambas <firstname.lastname@example.org> date: Fri Jul 01 22:22:18 2011 +0200 summary: bug 658894 - Collect basic telemetry for HTTP requests and page load. r=jduell
I'll take a look.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
There was just a single check for non-null missing - responseStart. It is strange we don't get it when we have responseEnd..
Attachment #550150 - Flags: review?(jduell.mcbugs)
Ah, I do understand. When the connection is closed by force or the request failed before any response bytes has been read then we mark responseEnd in nsHttpTransaction::Close but never mark responseStart that is done in nsHttpTransaction::WritePipeSegment (when data are avail on the socket). That is actually by the spec, it says to mark responseEnd even there were a connection failure and no data received. Should we suggest to mark also responseStart under the same conditions?
To avoid this bug, can't we just avoid the timestamp comparison in that case? I don't know if we need to change the spec for this...
(In reply to comment #5) > To avoid this bug, can't we just avoid the timestamp comparison in that > case? Sure, that is what the patch does. > I don't know if we need to change the spec for this... It has already changed. I found that out a minute after I posted the comment. My objection to the spec was that it doesn't make sense to measure just responseEnd w/o marking also responseStart. Do both or neither. Now the spec says "neither". And the concern was not about telemetry but about web timing API that is also based on the channel timing API.
Review ping. This needs to land before we can land bug 673228.
Jason: the patch is pretty trivial. Just removed read of a stamp we never used and added one more check that was missing.
Sent to try: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=626e32292fdc Assuming all green, will push to inbound after. I'm happy to fix it this time, but for the future please can you make sure the format (context, commit message, author) is correct: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed Thanks :-)
Whiteboard: [orange] → [orange] [inbound]
(In reply to Ed Morley [:edmorley] from comment #9) > I'm happy to fix it this time, but for the future please can you make sure > the format (context, commit message, author) is correct: > http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed > > Thanks :-) I usually commit my patches my self and update it correctly during push.
Ah I see someone else set the checkin-needed :-)
Whiteboard: [orange] [inbound] → [orange]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.