Null timestamp usage in telemetry code

RESOLVED FIXED in mozilla9

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: mayhemer)

Tracking

({intermittent-failure})

unspecified
mozilla9
x86
Mac OS X
intermittent-failure
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Here's the log:

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1311272451.1311274479.16733.gz&fulltext=1#err0
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
Blocks: 438871
Whiteboard: [orange]
(Assignee)

Comment 2

6 years ago
I'll take a look.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
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..
Attachment #550150 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 4

6 years ago
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...
(Assignee)

Updated

6 years ago
Depends on: 662555
(Assignee)

Comment 6

6 years ago
(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.
(Reporter)

Updated

6 years ago
Blocks: 673228
(Reporter)

Comment 7

6 years ago
Review ping. This needs to land before we can land bug 673228.
(Assignee)

Comment 8

6 years ago
Jason: the patch is pretty trivial.  Just removed read of a stamp we never used and added one more check that was missing.
Attachment #550150 - Flags: review?(jduell.mcbugs) → review+
Keywords: checkin-needed
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 :-)
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3679628bee5
Keywords: checkin-needed
Whiteboard: [orange] → [orange] [inbound]
Target Milestone: --- → mozilla9
(Assignee)

Comment 11

6 years ago
(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 :-)
http://hg.mozilla.org/mozilla-central/rev/c3679628bee5
Whiteboard: [orange] [inbound] → [orange]

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Keywords: intermittent-failure
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.