673226 - Null timestamp usage in telemetry code

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Here's the log:

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1311272451.1311274479.16733.gz&fulltext=1#err0

Comment 1

[Justin Lebar (not reading bugmail)]

The asserting code is

changeset:   72136:eb5866601f88
user:        Honza Bambas <[e-mail]>
date:        Fri Jul 01 22:22:18 2011 +0200
summary:     bug 658894 - Collect basic telemetry for HTTP requests and page load. r=jduell

Comment 2

[Honza Bambas (:mayhemer)]

I'll take a look.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

There was just a single check for non-null missing - responseStart.  It is strange we don't get it when we have responseEnd..

Comment 4

[Honza Bambas (:mayhemer)]

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?

Comment 5

[(no longer active)]

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...

Comment 6

[Honza Bambas (:mayhemer)]

(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.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Review ping. This needs to land before we can land bug 673228.

Comment 8

[Honza Bambas (:mayhemer)]

Jason: the patch is pretty trivial.  Just removed read of a stamp we never used and added one more check that was missing.

Comment 9

[Ed Morley [:emorley]]

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 :-)

Comment 10

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c3679628bee5

Comment 11

[Honza Bambas (:mayhemer)]

(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.

Comment 12

[Ed Morley [:emorley]]

Ah I see someone else set the checkin-needed :-)

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/c3679628bee5