New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 798446 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

TABs should be ignored while parsing certain headers

Reported by cva...@akamai.com, Jan 2 2018

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36

Steps to reproduce the problem:
1. With Chrome 65 (Canary at present), go to https://cvazac.netlify.com/tabs.html

What is the expected behavior?
The asserts should succeed, that is, the duration of the server timing entry should be 123.

What went wrong?
We get back 0 as a duration because TABs aren't treated as legal whitespace here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/network/HeaderFieldTokenizer.cpp?q=CONSUMEtokenor&sq=package:chromium&l=130

Did this work before? No 

Does this work in other browsers? N/A

Chrome version: 63.0.3239.108  Channel: stable
OS Version: OS X 10.12.6
Flash Version: 

This also affects parsing of the Content-Type header.
 
Labels: Needs-Triage-M63
Components: Blink>PerformanceAPIs
Labels: OS-Chrome OS-Linux OS-Windows

Comment 3 by y...@yoav.ws, Jan 3 2018

Cc: y...@yoav.ws
Components: -Blink>PerformanceAPIs Blink>PerformanceAPIs>ServerTiming
Labels: Triaged-ET Needs-Feedback
Tested the issue on chrome version 65.0.3325.73 on Ubuntu 14.04 with steps mentioned below:
1) Launched chrome and navigated to URL: https://cvazac.netlify.com/tabs.html
2) Opened Devtools> Console and Devtools> Network and checked for the time field
3) In Devtools> Network tab, under time section observed 27ms 

@Reporter:
Please find the attached screenshot for your reference and let us know your inputs on it which helps us in further triaging it in better way.

Thanks!
798446.png
159 KB View Download

Comment 6 by cva...@akamai.com, Feb 21 2018

Switch to the console and you will see the failing assert. 

If you evaluate this in the console, the value will be 0. It should be 123.
```
performance.getEntriesByType('resource')[0].serverTiming[0].duration
```

If you switch over to the network tab, you'll see that the response header is this:
```
server-timing: metric	;	dur	=	123	;
```
Project Member

Comment 7 by sheriffbot@chromium.org, Feb 21 2018

Cc: viswa.karala@chromium.org
Labels: -Needs-Feedback
Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: dproy@chromium.org
Status: Assigned (was: Unconfirmed)
This looks extremely straight forward.

Deep, can you take a look?
http://jsonbin.org was a useful test example, but their header format is deprecated (as the console message mentions).  I have an internal test that adds the Server-Timing header (regrettably I can't expose it), but I get the output in dev tools (in screenshot illustrated as "pure_delay" = 506.5ms), but nothing in the resourceTiming entry (blank entry in serverTiming).

Since the previous release (macOS chrome 65), the values would appear in devTools, but no duration was included in the resourceTiming entry.  These current examples are from "66.0.3359.117 (Official Build)"

serverTimingIllustration.png
166 KB View Download
Owner: ----
Status: Available (was: Assigned)
Owner: tommckee@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5ec3a7338a79dc825bb2bde3efeeb056ff117f06

commit 5ec3a7338a79dc825bb2bde3efeeb056ff117f06
Author: Tom McKee <tommckee@chromium.org>
Date: Mon Jul 23 23:38:45 2018

Adding tests against and fixes for tabs in Server-Timing headers

According to the spec
(https://w3c.github.io/server-timing/#the-server-timing-header-field),
optional whitespace is allowed amongst parameter names and values. We
should be discarding this whitespace during parsing but, when there are
tabs, we were treating it like an error and discarding the whole value.

Changed the code that tokenizes HTTP headers to skip tabs and spaces
when consuming optional whitespace.

BUG= 798446 

Change-Id: If776761e5ea199e662ec7b2b5aa245e4581131fd
Reviewed-on: https://chromium-review.googlesource.com/1147258
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Tom McKee <tommckee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577325}
[add] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/parsing/84.js
[add] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/WebKit/LayoutTests/external/wpt/server-timing/resources/parsing/84.js.sub.headers
[modify] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/WebKit/LayoutTests/external/wpt/server-timing/server_timing_header-parsing-expected.txt
[modify] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/WebKit/LayoutTests/external/wpt/server-timing/server_timing_header-parsing.html
[modify] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/WebKit/LayoutTests/external/wpt/server-timing/server_timing_header-parsing.https-expected.txt
[modify] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/WebKit/LayoutTests/external/wpt/server-timing/server_timing_header-parsing.https.html
[modify] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/blink/renderer/platform/network/header_field_tokenizer.cc
[modify] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/blink/renderer/platform/network/header_field_tokenizer.h
[modify] https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06/third_party/blink/renderer/platform/network/parsed_content_header_field_parameters_test.cc

Status: Fixed (was: Assigned)
Fixed in https://crrev.com/5ec3a7338a79dc825bb2bde3efeeb056ff117f06

Sign in to add a comment