Preserve custom headers when following cross-origin redirects.

Preserve custom headers when following cross-origin redirects.

BUG=661782
Committed: https://crrev.com/75318e021b295e4285017b661f7cf9ae41efc85d
Cr-Commit-Position: refs/heads/master@{#433421}

[Jack Bates, 2016-11-02 22:36:07]

Description was changed from

==========
Preserve custom headers when following cross-origin redirects.

BUG=661782
==========

to

==========
Preserve custom headers when following cross-origin redirects.

BUG=661782
==========

[Jack Bates, 2016-11-02 22:36:07]

jack@nottheoilrig.com changed reviewers:
+ yhirano@chromium.org

[Jack Bates, 2016-11-03 17:44:28]

Patchset #1 (id:1) has been deleted

[yhirano, 2016-11-04 02:24:02]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 02:25:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 03:58:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 03:58:05]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-11-07 07:48:30]

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
(right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:143:

Please add a test to verify that ACAOrigin and ACAHeaders are really needed.

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:221:
HTTPHeaderMap m_requestHeaders;
Do we still need this? Is it enough to use the headers on the original request?

[tyoshino (SeeGerritForStatus), 2016-11-07 10:48:17]

tyoshino@chromium.org changed reviewers:
+ tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-11-07 10:48:17]

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:221:
HTTPHeaderMap m_requestHeaders;
On 2016/11/07 07:48:30, yhirano wrote:
> Do we still need this? Is it enough to use the headers on the original
request?

Yeah, it's good to try it to see if just depending on |request| argument of
redirectReceived() works. But don't we need to recover the 3 headers cleared in
redirectReceived() anyway? Or, just also removing all the clearHTTP.*() calls
would be fine?

[yhirano, 2016-11-07 12:30:50]

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:205: const
HTTPHeaderMap& headerMap = request.httpHeaderFields();
m_requestHeaders = request.httpHeaderFields();

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:221:
HTTPHeaderMap m_requestHeaders;
On 2016/11/07 10:48:17, tyoshino wrote:
> On 2016/11/07 07:48:30, yhirano wrote:
> > Do we still need this? Is it enough to use the headers on the original
> request?
> 
> Yeah, it's good to try it to see if just depending on |request| argument of
> redirectReceived() works. But don't we need to recover the 3 headers cleared
in
> redirectReceived() anyway? Or, just also removing all the clearHTTP.*() calls
> would be fine?

Ah, you're right, thank you.

[Jack Bates, 2016-11-12 20:57:13]

Patchset #2 (id:40001) has been deleted

[Jack Bates, 2016-11-12 21:13:30]

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js
(right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/redirect.js:143:

On 2016/11/07 07:48:30, yhirano wrote:
> Please add a test to verify that ACAOrigin and ACAHeaders are really needed.

Done.

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:205: const
HTTPHeaderMap& headerMap = request.httpHeaderFields();
On 2016/11/07 12:30:50, yhirano wrote:
> m_requestHeaders = request.httpHeaderFields();

Done.

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:221:
HTTPHeaderMap m_requestHeaders;
On 2016/11/07 12:30:50, yhirano wrote:
> On 2016/11/07 10:48:17, tyoshino wrote:
> > On 2016/11/07 07:48:30, yhirano wrote:
> > > Do we still need this? Is it enough to use the headers on the original
> > request?
> > 
> > Yeah, it's good to try it to see if just depending on |request| argument of
> > redirectReceived() works. But don't we need to recover the 3 headers cleared
> in
> > redirectReceived() anyway? Or, just also removing all the clearHTTP.*()
calls
> > would be fine?
> 
> Ah, you're right, thank you.

Do you have any tips on how to inspect the HTTPHeaderMap object?

> (gdb) p request.m_httpHeaderFields
> $1 = {m_headers = {
>     m_impl = {<(anonymous
namespace)::ConditionalDestructor<WTF::HashTable<WTF::AtomicString,
WTF::KeyValuePair<WTF::AtomicString, > WTF::AtomicString>,
WTF::KeyValuePairKeyExtractor, WTF::CaseFoldingHash,
WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>,
WTF::HashTraits<WTF::AtomicString> >, WTF::HashTraits<WTF::AtomicString>,
WTF::PartitionAllocator>, false>> = {<No data fields>}, 
>       static m_maxLoad = 2, static m_minLoad = 6, m_table = 0x3e29d3020070, 
>       m_tableSize = 8, m_keyCount = 2, m_deletedCount = 0, m_queueFlag = 0, 
>       m_accessForbidden = 0, m_modifications = 2}}}
> (gdb)

[yhirano, 2016-11-14 08:42:43]

lgtm, thanks.

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/2471533005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h:221:
HTTPHeaderMap m_requestHeaders;
On 2016/11/12 21:13:30, Jack Bates wrote:
> On 2016/11/07 12:30:50, yhirano wrote:
> > On 2016/11/07 10:48:17, tyoshino wrote:
> > > On 2016/11/07 07:48:30, yhirano wrote:
> > > > Do we still need this? Is it enough to use the headers on the original
> > > request?
> > > 
> > > Yeah, it's good to try it to see if just depending on |request| argument
of
> > > redirectReceived() works. But don't we need to recover the 3 headers
cleared
> > in
> > > redirectReceived() anyway? Or, just also removing all the clearHTTP.*()
> calls
> > > would be fine?
> > 
> > Ah, you're right, thank you.
> 
> Do you have any tips on how to inspect the HTTPHeaderMap object?
> 
> > (gdb) p request.m_httpHeaderFields
> > $1 = {m_headers = {
> >     m_impl = {<(anonymous
> namespace)::ConditionalDestructor<WTF::HashTable<WTF::AtomicString,
> WTF::KeyValuePair<WTF::AtomicString, > WTF::AtomicString>,
> WTF::KeyValuePairKeyExtractor, WTF::CaseFoldingHash,
> WTF::HashMapValueTraits<WTF::HashTraits<WTF::AtomicString>,
> WTF::HashTraits<WTF::AtomicString> >, WTF::HashTraits<WTF::AtomicString>,
> WTF::PartitionAllocator>, false>> = {<No data fields>}, 
> >       static m_maxLoad = 2, static m_minLoad = 6, m_table = 0x3e29d3020070, 
> >       m_tableSize = 8, m_keyCount = 2, m_deletedCount = 0, m_queueFlag = 0, 
> >       m_accessForbidden = 0, m_modifications = 2}}}
> > (gdb) 

I don't know. You can print hashmap entries like


request.m_httpHeaderFields.m_headers.m_impl.m_table[2].key.m_string.utf8(WTF::LenientUTF8Conversion).data()

, but it's not that user-friendly.

[Jack Bates, 2016-11-18 16:49:21]

The CQ bit was checked by jack@nottheoilrig.com

[commit-bot: I haz the power, 2016-11-18 16:50:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 21:41:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 21:41:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jack Bates, 2016-11-18 21:52:59]

The CQ bit was checked by jack@nottheoilrig.com

[commit-bot: I haz the power, 2016-11-18 21:53:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 22:36:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-18 22:37:00]

Failed to apply patch for AUTHORS:
While running git apply --index -p1;
  error: patch failed: AUTHORS:279
  error: AUTHORS: patch does not apply

Patch:       AUTHORS
Index: AUTHORS
diff --git a/AUTHORS b/AUTHORS
index
93a5671e28201e5d241712adefaf5e0537efe3ba..61ac28aa2a2c2d5a384905e206418dba695edcd4
100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -279,6 +279,7 @@ Ion Rosca <rosca@adobe.com>
 Isaac Reilly <reillyi@amazon.com>
 Ivan Sham <ivansham@amazon.com>
 J. Ryan Stinnett <jryans@chromium.org>
+Jack Bates <jack@nottheoilrig.com>
 Jacob Mandelson <jacob@mandelson.org>
 Jaehun Lim <ljaehun.lim@samsung.com>
 Jaekyeom Kim <btapiz@gmail.com>

[Jack Bates, 2016-11-19 15:25:47]

The CQ bit was checked by jack@nottheoilrig.com

[Jack Bates, 2016-11-19 15:25:48]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2471533005/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-11-19 15:25:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 17:48:03]

Description was changed from

==========
Preserve custom headers when following cross-origin redirects.

BUG=661782
==========

to

==========
Preserve custom headers when following cross-origin redirects.

BUG=661782
==========

[commit-bot: I haz the power, 2016-11-19 17:48:05]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-11-19 17:50:35]

Description was changed from

==========
Preserve custom headers when following cross-origin redirects.

BUG=661782
==========

to

==========
Preserve custom headers when following cross-origin redirects.

BUG=661782

Committed: https://crrev.com/75318e021b295e4285017b661f7cf9ae41efc85d
Cr-Commit-Position: refs/heads/master@{#433421}
==========

[commit-bot: I haz the power, 2016-11-19 17:50:37]

Patchset 3 (id:??) landed as
https://crrev.com/75318e021b295e4285017b661f7cf9ae41efc85d
Cr-Commit-Position: refs/heads/master@{#433421}