Null timestamp usage in telemetry code

RESOLVED FIXED in mozilla9

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jrmuizel, Assigned: mayhemer)

Tracking

({intermittent-failure})

unspecified
mozilla9
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

Updated

8 years ago
Blocks: 438871
Whiteboard: [orange]
Assignee

Comment 2

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

Comment 3

8 years ago
Posted patch v1Splinter Review
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

8 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?

Comment 5

8 years ago
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

8 years ago
Depends on: 662555
Assignee

Comment 6

8 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

8 years ago
Blocks: 673228
Reporter

Comment 7

8 years ago
Review ping. This needs to land before we can land bug 673228.
Assignee

Comment 8

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

Updated

8 years ago
Attachment #550150 - Flags: review?(jduell.mcbugs) → review+

Updated

8 years ago
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 :-)
Target Milestone: --- → mozilla9
Assignee

Comment 11

8 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 :-)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.