Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1378543002:
Add UMA for header values in XHR's setRequestHeader() checked against RFC 7230 (Closed)
DescriptionAdd UMA for header values in XHR's setRequestHeader() checked against RFC 7230
Previously, deprecation warnings were shown for the following three kinds of
header values:
Category 1. Header values with control octets that are rejected by RFC 7230,
Category 2. Header values with leading/trailing whitespaces,
but normalization doesn't have effect (because other parts of Chromium
strip such whitespaces before sent to network), and
Category 3. Header values with leading/trailing whitespaces
and normalization does have effect. For example, in the case of calling
setRequestHeader('test', 'a ') and setRequestHeader('test', ' b'),
the whitespaces remains in the middle of the header value ('test: a , b')
and will be removed by normalization.
This CL suppresses deprecation warnings for all of them and adds UMA instead:
HeaderValueInvalid: Category 1.
HeaderValueAffectedByNormalization: Category 3.
HeaderValueValid: Category 2 and header values valid in RFC 7230.
BUG=455099
Committed: https://crrev.com/ce71fe405f5df954732e01eb88627e8843a3a587
Cr-Commit-Position: refs/heads/master@{#352247}
Patch Set 1 #Patch Set 2 : Update test expectations due to message change. #
Total comments: 10
Patch Set 3 : Reflected tyoshino's comments. #
Total comments: 3
Patch Set 4 : Use UMA without deprecation warnings. #
Total comments: 4
Patch Set 5 : Reflected comments. #
Messages
Total messages: 33 (11 generated)
hiroshige@chromium.org changed reviewers: + shiva.jm@samsung.com, tyoshino@chromium.org
hiroshige@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko, could you take a look as core/ OWNER?
On 2015/09/29 05:39:24, hiroshige wrote: > +kinuko, could you take a look as core/ OWNER? Context: this CL is migrated from https://codereview.chromium.org/1342443004/ due to repository merge.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1378543002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1378543002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/invalid-octets.html (right): https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/invalid-octets.html:4: <script src = "../../resources/testharnessreport.js"></script> Majority of the files in this directly are using /js-test-resources/ Align with them? https://code.google.com/p/chromium/codesearch#search/&q=%5C.%5C./%5C.%5C./%20... https://code.google.com/p/chromium/codesearch#search/&q=js-test-resources%20f... https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/invalid-octets.html:19: 'Whitespaces remains in the middle of the header value.'); Whitespace -> Control octets? https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html (right): https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html:8: // Normalization will remove leading/trailing whitespaces and has effects Please explain more background. This comment is hard to understand without background. E.g. According to the spec, normalization (removing leading/trailing whitespaces) must be done on the value argument on setRequestHeader() call. Blink's Fetch API implementation doesn't do that currently but Chromium's network stack is already doing that. So, whether Blink does the normalization or not doesn't have any effect on the data sent to network. But the normalization by Chromium is made on the result of value concatenation by Blink. Blah, Blah, ... https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html:30: done(); is this done() needed? https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-unaffected.html (right): https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-unaffected.html:16: console.error('This error is added to avoid presubmit errors for -expected.txt with no CONSOLE WARNING. This is testing that no CONSOLE WARNING is shown for setRequestHeader().'); hmm, as the expectation doesn't contain anything but the template output of testharness, can't we just omit the expectation file?
https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/invalid-octets.html (right): https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/invalid-octets.html:4: <script src = "../../resources/testharnessreport.js"></script> On 2015/09/29 09:02:24, tyoshino wrote: > Majority of the files in this directly are using /js-test-resources/ > > Align with them? > > https://code.google.com/p/chromium/codesearch#search/&q=%5C.%5C./%5C.%5C./%20... > https://code.google.com/p/chromium/codesearch#search/&q=js-test-resources%20f... Done. https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/invalid-octets.html:19: 'Whitespaces remains in the middle of the header value.'); On 2015/09/29 09:02:24, tyoshino wrote: > Whitespace -> Control octets? Done. https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html (right): https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html:8: // Normalization will remove leading/trailing whitespaces and has effects On 2015/09/29 09:02:24, tyoshino wrote: > Please explain more background. This comment is hard to understand without > background. E.g. > > According to the spec, normalization (removing leading/trailing whitespaces) > must be done on the value argument on setRequestHeader() call. Blink's Fetch API > implementation doesn't do that currently but Chromium's network stack is already > doing that. So, whether Blink does the normalization or not doesn't have any > effect on the data sent to network. But the normalization by Chromium is made on > the result of value concatenation by Blink. Blah, Blah, ... Done. https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html:30: done(); On 2015/09/29 09:02:24, tyoshino wrote: > is this done() needed? Done (moved to js-test) https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-unaffected.html (right): https://codereview.chromium.org/1378543002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-unaffected.html:16: console.error('This error is added to avoid presubmit errors for -expected.txt with no CONSOLE WARNING. This is testing that no CONSOLE WARNING is shown for setRequestHeader().'); On 2015/09/29 09:02:24, tyoshino wrote: > hmm, as the expectation doesn't contain anything but the template output of > testharness, can't we just omit the expectation file? This workaround is removed because the test is moved to js-test-based. FYI the answer for the question is No, this test checks NO deprecation warning is output (and if we omit the expectation file we don't check that).
Some documentation nits, code changes (in core/) lgtm https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html (right): https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/header-value-update/normalize-affected-1.html:36: // a3 and b3 are different, so a deprecation warning should be shown. Wow super detailed comments. I feel a bit shorter comment like below would probably work, but I defer decision to tyoshino@ and you. (Either works for me) "In the following case Chromium's new normalization code will normalize 'a, \tb' into 'a, b', while current code doesn't remove leading/trailing whitespaces (i.e. 'a, \tb' is kept as is), so a deprecation warning should be shown." https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.cpp:936: return "According to the Fetch Standard, setRequestHeader() must strip leading/trailing whitespaces, but it doesn't currently. We are experimenting whether we can make it conform to the spec (see: https://www.chromestatus.com/feature/6457425448140800). This script will be affected by the change."; "We are experimenting whether..." the intention of these sentences' are unclear, do we expect developers to change the code so that it own't be affected by the potential future change? If so probably just writing it would be better. (E.g. "... doesn't currently. The current behavior might be changed to conform to the spec (see https://..../) and your code will be affected by the change.")
https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/UseCounter.cpp:936: return "According to the Fetch Standard, setRequestHeader() must strip leading/trailing whitespaces, but it doesn't currently. We are experimenting whether we can make it conform to the spec (see: https://www.chromestatus.com/feature/6457425448140800). This script will be affected by the change."; On 2015/09/29 12:51:33, kinuko wrote: > "We are experimenting whether..." the intention of these sentences' are unclear, > do we expect developers to change the code so that it own't be affected by the > potential future change? "Just experimenting" is somehow accurate: we want to collect stats, became not sure whether we can actually do the change (due to expanding the scope to //net as a result of discussions). Should we turn this stats into UMAs (without warnings), if still exposing these deprecation warnings to developers is confusing?
On 2015/09/29 13:21:38, hiroshige wrote: > https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): > > https://codereview.chromium.org/1378543002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/UseCounter.cpp:936: return "According to > the Fetch Standard, setRequestHeader() must strip leading/trailing whitespaces, > but it doesn't currently. We are experimenting whether we can make it conform to > the spec (see: https://www.chromestatus.com/feature/6457425448140800). This > script will be affected by the change."; > On 2015/09/29 12:51:33, kinuko wrote: > > "We are experimenting whether..." the intention of these sentences' are > unclear, > > do we expect developers to change the code so that it own't be affected by the > > potential future change? > "Just experimenting" is somehow accurate: we want to collect stats, became not > sure whether we can actually do the change (due to expanding the scope to //net > as a result of discussions). > > Should we turn this stats into UMAs (without warnings), if still exposing these > deprecation warnings to developers is confusing? Deprecation warnings is usually used to communicate with developers, if that's not really the intention showing message may not make much sense.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1378543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1378543002/60001
Patch Set 4: Created UMA-only CL (no warnings for developers). PTAL.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hiroshige@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for tools/metrics/ OWNER. Could you take a look? Also updated CL description.
https://codereview.chromium.org/1378543002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1378543002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49647: +<histogram name="WebCore.XHR.setRequestHeader.HeaderValueCategoryInRFC7230" Would it make more sense for this to be Blink.* instead of WebCore.*? https://codereview.chromium.org/1378543002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:73823: + <int value="0" label="XMLHttpRequestHeaderValueInvalid"/> These are human readable, so suggest more easier to read names - e.g. Header Invalid, Header Valid, etc.
https://codereview.chromium.org/1378543002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1378543002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49647: +<histogram name="WebCore.XHR.setRequestHeader.HeaderValueCategoryInRFC7230" On 2015/09/30 19:44:45, Alexei Svitkine wrote: > Would it make more sense for this to be Blink.* instead of WebCore.*? Done. https://codereview.chromium.org/1378543002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:73823: + <int value="0" label="XMLHttpRequestHeaderValueInvalid"/> On 2015/09/30 19:44:45, Alexei Svitkine wrote: > These are human readable, so suggest more easier to read names - e.g. Header > Invalid, Header Valid, etc. Done.
lgtm
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/1378543002/#ps80001 (title: "Reflected comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1378543002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1378543002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1378543002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1378543002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ce71fe405f5df954732e01eb88627e8843a3a587 Cr-Commit-Position: refs/heads/master@{#352247} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
