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

Issue 763804 link

Starred by 1 user
Status: Verified
Owner:
█ tsniatow...@opera.com
Email to this user bounced
Closed: Sep 2017
Cc:
jam@chromium.org
caseq@chromium.org
dgozman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

PlzNavigate: passing multiple extra_headers in LoadURLParams doesn't work

Reported by tsniatow...@opera.com, Sep 11 2017 
Chrome Version: master@Sep10 (d88a223e383cc4b24fcd137d28a8126d2efc11a2)

What steps will reproduce the problem?
(1) End up calling NavigationControllerImpl::LoadURLWithParams 
(2) Have more than one extra header in params.extra_headers, separated by \n as per the docs for LoadURLParams
(3) Have PlzNavigate enabled

What is the expected result?
All passed extra_headers are sent.

What happens instead?
DFATAL in http_request_headers.cc(146), or zero headers work (in non-dcheck builds).



This is likely caused by a confusion as to whether the single-string headers should be passes \n-separated, or \r\n-separated. LoadURLParams says they should be \n-separated, NavigationEntryImpl wants then \r\n-separated. Outside PlzNavigate, enough data-conversion happens on the trip back from Blink to ensure they are \r\n anyway by the time they get to http_request_headers.cc, but the shorted PlzNavigate path fails to account for that difference.

The fix would be to either make LoadUrlParams also assume a \r\n-separated list of headers, or fix NavigationControllerImpl so it converts. I'm trying the latter as it looks like less code needs to change.

Note there doesn't seem to be any unit or browser tests that try to pass more than one extra_header in LoadURLParams.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 11 2017 

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

commit 11fec36039b14d99c7eb937eb034281f6fa854da
Author: tsniatowski@opera.com <tsniatowski@opera.com>
Date: Mon Sep 11 14:06:50 2017

PlzNavigate: Fix multiple extra_headers in LoadUrlWithParams

NavigationControllerImpl operates on LoadURLParams, which state
"Extra headers for this load, separated by \n.", and on
NavigationEntryImpl, which state "Extra headers (separated by \r\n)
to send during the request". However, there is no conversion
from one form to the other, instead the extra headers are passed
verbatim.

This is not an issue when PlzNavigate is not enabled, because there
are more conversions on the way to and from the renderer, and things
end up working anyway. However, the shorter path of PlzNavigate
makes code further down choke on improper data when more than one
header is passed.

Fix by converting LF to CRLF when moving from LoadURLParams to
NavigationEntryImpl, and by adding a second header to a bunch of tests
that only sent one extra_header, thus not showing the problem.

BUG= 763804 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I3e492520c8bd059b0d00107b38cfdf6daa8d96f2
Reviewed-on: https://chromium-review.googlesource.com/659577
Commit-Queue: Tomasz Sniatowski <tsniatowski@opera.com>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500905}
[modify] https://crrev.com/11fec36039b14d99c7eb937eb034281f6fa854da/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/11fec36039b14d99c7eb937eb034281f6fa854da/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/11fec36039b14d99c7eb937eb034281f6fa854da/content/browser/frame_host/navigation_controller_impl_unittest.cc

Comment 2 by tsniatow...@opera.com, Sep 14 2017

Status: Fixed (was: Untriaged)

Comment 3 by tsniatow...@opera.com, Sep 14 2017

Status: Verified (was: Fixed)
►

Sign in to add a comment