[Service Workers] DCHECK from WebServiceWorkerResponse::visitHTTPHeaderFields.

[Service Workers] DCHECK from WebServiceWorkerResponse::visitHTTPHeaderFields.

Headers are ISO 8859-1 (also called ISO Latin 1), and not ASCII, any attempt of
|base::UTF16ToASCII| may result into DCHECK.

This patch fixes the issue by replacing |base::UTF16ToASCII| with
|blink::WebString.latin1()| API.

BUG=537127
TEST=serviceworker/iso-latin1-header.html

Committed: https://crrev.com/348710b1cacbef1d1cd7057be134fa56cd34fdab
Cr-Commit-Position: refs/heads/master@{#353700}

[Pritam Nikam, 2015-09-29 12:39:08]

Patchset #1 (id:1) has been deleted

[Pritam Nikam, 2015-09-29 12:39:58]

pritam.nikam@samsung.com changed reviewers:
+ falken@chromium.org

[Pritam Nikam, 2015-09-29 12:39:58]

Hi falken,

PTAL, thanks!

[falken, 2015-10-01 12:20:09]

falken@chromium.org changed reviewers:
+ horo@chromium.org, tyoshino@chromium.org

[falken, 2015-10-01 12:20:10]

Sorry for the late review and thanks for working on this.

I checked codesearch and some subclasses of WebHTTPHeaderVisitor use latin1()
and some use utf8(). I'm not confident enough to review. 

horo@, tyoshino@ could you look?

[horo, 2015-10-02 09:34:22]

Looks good.
But could you please add LayoutTest like
LayoutTests/http/tests/serviceworker/invalid-header.html?
I think "event.respondWith(new Response('', {headers: {TEST: 'À'}}));" can
reproduce this DCHECK failure.

[tyoshino (SeeGerritForStatus), 2015-10-02 10:56:37]

On 2015/10/01 12:20:10, falken wrote:
> Sorry for the late review and thanks for working on this.
> 
> I checked codesearch and some subclasses of WebHTTPHeaderVisitor use latin1()
> and some use utf8(). I'm not confident enough to review. 
> 
> horo@, tyoshino@ could you look?

Yeah. For example, when a request is sent from Blink to Chromium,
WebURLLoaderImpl converts WebURLRequest into RequestInfo (and then, into IPC)
using HeaderFlattener + WebURLRequest::visitHTTPHeaderFields(). HeaderFlattener
uses latin1().

Unless the classes are intended to handle only ASCII data, this should be
latin1() call.

lgtm

[Pritam Nikam, 2015-10-12 11:36:31]

On 2015/10/02 10:56:37, tyoshino wrote:
> On 2015/10/01 12:20:10, falken wrote:
> > Sorry for the late review and thanks for working on this.
> > 
> > I checked codesearch and some subclasses of WebHTTPHeaderVisitor use
latin1()
> > and some use utf8(). I'm not confident enough to review. 
> > 
> > horo@, tyoshino@ could you look?
> 
> Yeah. For example, when a request is sent from Blink to Chromium,
> WebURLLoaderImpl converts WebURLRequest into RequestInfo (and then, into IPC)
> using HeaderFlattener + WebURLRequest::visitHTTPHeaderFields().
HeaderFlattener
> uses latin1().
> 
> Unless the classes are intended to handle only ASCII data, this should be
> latin1() call.
> 
> lgtm

Thank you all for review!
Sorry for delay, I was away. I'll add layout tests in new patch set.

Thanks!

[Pritam Nikam, 2015-10-12 14:05:25]

Hi,

Uploaded new patch with layout tests, ptal.
Thanks!

[horo, 2015-10-13 04:33:53]

lgtm.

Thank you! :)

[Pritam Nikam, 2015-10-13 05:14:55]

On 2015/10/13 04:33:53, horo wrote:
> lgtm.
> 
> Thank you! :)

Thanks horo,
I'll proceed with commit.
Thanks!

[Pritam Nikam, 2015-10-13 05:14:59]

The CQ bit was checked by pritam.nikam@samsung.com

[Pritam Nikam, 2015-10-13 05:15:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org
Link to the patchset: https://codereview.chromium.org/1372423002/#ps40001
(title: "Added LayoutTest.")

[commit-bot: I haz the power, 2015-10-13 05:15:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372423002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372423002/40001

[tyoshino (SeeGerritForStatus), 2015-10-13 05:57:06]

https://codereview.chromium.org/1372423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iso-latin1-header-iframe.html
(right):

https://codereview.chromium.org/1372423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iso-latin1-header-iframe.html:12:
reject('XHR must fail.');
fail -> succeed

[commit-bot: I haz the power, 2015-10-13 07:21:45]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-10-13 07:22:39]

Patchset 2 (id:??) landed as
https://crrev.com/348710b1cacbef1d1cd7057be134fa56cd34fdab
Cr-Commit-Position: refs/heads/master@{#353700}

[Pritam Nikam, 2015-10-13 08:13:01]

Thanks tyoshino for code review! Your comments were addressed in follow-up CL:
  https://codereview.chromium.org/1403773004/

Thanks!

https://codereview.chromium.org/1372423002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iso-latin1-header-iframe.html
(right):

https://codereview.chromium.org/1372423002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/iso-latin1-header-iframe.html:12:
reject('XHR must fail.');
On 2015/10/13 05:57:06, tyoshino wrote:
> fail -> succeed

Done.