Last Comment Bug 673226 - Null timestamp usage in telemetry code
: Null timestamp usage in telemetry code
Status: RESOLVED FIXED
: intermittent-failure
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on: 662555
Blocks: 438871 673228
  Show dependency treegraph
 
Reported: 2011-07-21 13:57 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-11-25 19:31 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.45 KB, patch)
2011-08-02 12:01 PDT, Honza Bambas (:mayhemer)
jduell.mcbugs: review+
Details | Diff | Review

Comment 1 Justin Lebar (not reading bugmail) 2011-07-21 14:04:34 PDT
The asserting code is

changeset:   72136:eb5866601f88
user:        Honza Bambas <honzab.moz@firemni.cz>
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) 2011-07-21 14:48:49 PDT
I'll take a look.
Comment 3 Honza Bambas (:mayhemer) 2011-08-02 12:01:52 PDT
Created attachment 550150 [details] [diff] [review]
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) 2011-08-02 12:06:06 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-02 12:25:49 PDT
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) 2011-08-02 12:29:51 PDT
(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] 2011-08-10 19:23:07 PDT
Review ping. This needs to land before we can land bug 673228.
Comment 8 Honza Bambas (:mayhemer) 2011-08-12 04:50:50 PDT
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] 2011-08-20 15:57:19 PDT
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 11 Honza Bambas (:mayhemer) 2011-08-21 06:40:30 PDT
(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] 2011-08-21 06:43:50 PDT
Ah I see someone else set the checkin-needed :-)

Note You need to log in before you can comment on or make changes to this bug.