WindowBase64::atob() does not remove space characters from input

WindowBase64::atob() does not remove space characters from input

WindowBase64::atob() does not remove space characters from input as mandated
by the specification (Step 3):
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#dom-windowbase64-atob

As a result, base64 decoding will fail if the input string contains HTML
space characters.

This CL makes our implementation more flexible so that it allows HTML spaces
in the encoded input, which is apparently desired because quite common:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22731

We now have a 100% pass rate on the following test suite:
http://www.w3c-test.org/html/tests/submission/AryehGregor/base64.html

BUG=314682
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161395

[arv (Not doing code reviews), 2013-11-04 19:54:53]

https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
File Source/core/frame/DOMWindowBase64.cpp (right):

https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
Source/core/frame/DOMWindowBase64.cpp:69: if
(encodedString.find(isHTMLSpace<UChar>) != NotFoundError) {
Can we change base64Decode to do the right thing so we don't have to go through
the string twice?

Maybe add another entry to the the Base64DecodePolicy?

It seems a bit strange that we have 4 different policies. Based on what I see we
only use 3 of them and Base64FailOnInvalidCharacterOrExcessPadding is only used
by atob, so maybe we can change that?

[Inactive, 2013-11-04 20:01:37]

https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
File Source/core/frame/DOMWindowBase64.cpp (right):

https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
Source/core/frame/DOMWindowBase64.cpp:69: if
(encodedString.find(isHTMLSpace<UChar>) != NotFoundError) {
On 2013/11/04 19:54:53, arv wrote:
> Can we change base64Decode to do the right thing so we don't have to go
through
> the string twice?
> 
> Maybe add another entry to the the Base64DecodePolicy?
> 
> It seems a bit strange that we have 4 different policies. Based on what I see
we
> only use 3 of them and Base64FailOnInvalidCharacterOrExcessPadding is only
used
> by atob, so maybe we can change that?

We could indeed add an Base64DecodePolicy entry like
"FailOnOnInvalidCharacterOrExcessPaddingButAllowSpace" to do what we can in
atob(). The thing is that we would still need to go over the string twice.

Padding validation depends on the string length, and therefore assumes that
white spaces have already been removed in an earlier step. Also note that there
could be spaces in the padding itself which would be an issue as well for
validating the padding.

[tkent, 2013-11-05 02:21:56]

https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
File Source/core/frame/DOMWindowBase64.cpp (right):

https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
Source/core/frame/DOMWindowBase64.cpp:69: if
(encodedString.find(isHTMLSpace<UChar>) != NotFoundError) {
On 2013/11/04 20:01:37, Chris Dumez wrote:
> On 2013/11/04 19:54:53, arv wrote:
> > Can we change base64Decode to do the right thing so we don't have to go
> through
> > the string twice?
> > 
> > Maybe add another entry to the the Base64DecodePolicy?
> > 
> > It seems a bit strange that we have 4 different policies. Based on what I
see
> we
> > only use 3 of them and Base64FailOnInvalidCharacterOrExcessPadding is only
> used
> > by atob, so maybe we can change that?
> 
> We could indeed add an Base64DecodePolicy entry like
> "FailOnOnInvalidCharacterOrExcessPaddingButAllowSpace" to do what we can in
> atob(). The thing is that we would still need to go over the string twice.
> 
> Padding validation depends on the string length, and therefore assumes that
> white spaces have already been removed in an earlier step. Also note that
there
> could be spaces in the padding itself which would be an issue as well for
> validating the padding.

Ok, Implementation cost of the new flag looks high. It's ok to go ahead without
the new flag, and we should have a FIXME comment about the inefficiency.

Also, we can just do encodedString.removeCharacters() without
encodedString.find() because removeCharacters() doesn't update the StringImpl if
there are no matched characters.

[Inactive, 2013-11-05 17:22:05]

On 2013/11/05 02:21:56, tkent wrote:
>
https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
> File Source/core/frame/DOMWindowBase64.cpp (right):
> 
>
https://codereview.chromium.org/58143002/diff/1/Source/core/frame/DOMWindowBa...
> Source/core/frame/DOMWindowBase64.cpp:69: if
> (encodedString.find(isHTMLSpace<UChar>) != NotFoundError) {
> On 2013/11/04 20:01:37, Chris Dumez wrote:
> > On 2013/11/04 19:54:53, arv wrote:
> > > Can we change base64Decode to do the right thing so we don't have to go
> > through
> > > the string twice?
> > > 
> > > Maybe add another entry to the the Base64DecodePolicy?
> > > 
> > > It seems a bit strange that we have 4 different policies. Based on what I
> see
> > we
> > > only use 3 of them and Base64FailOnInvalidCharacterOrExcessPadding is only
> > used
> > > by atob, so maybe we can change that?
> > 
> > We could indeed add an Base64DecodePolicy entry like
> > "FailOnOnInvalidCharacterOrExcessPaddingButAllowSpace" to do what we can in
> > atob(). The thing is that we would still need to go over the string twice.
> > 
> > Padding validation depends on the string length, and therefore assumes that
> > white spaces have already been removed in an earlier step. Also note that
> there
> > could be spaces in the padding itself which would be an issue as well for
> > validating the padding.
> 
> Ok, Implementation cost of the new flag looks high. It's ok to go ahead
without
> the new flag, and we should have a FIXME comment about the inefficiency.
> 
> Also, we can just do encodedString.removeCharacters() without
> encodedString.find() because removeCharacters() doesn't update the StringImpl
if
> there are no matched characters.

Ok, I will investigate if I can do it more optimally in WTF. If not, I will add
a FIXME as proposed.

[Inactive, 2013-11-05 17:57:32]

Ok, I refactored the WTF implementation to support our use cases and avoid going
over the string twice.
Please take another look.

[arv (Not doing code reviews), 2013-11-05 18:13:13]

LGTM

[tkent, 2013-11-05 21:39:06]

lgtm with nits

https://codereview.chromium.org/58143002/diff/120001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/atob-btoa.html (right):

https://codereview.chromium.org/58143002/diff/120001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/atob-btoa.html:42: shouldBe('window.atob(" YQ==")',
'"a"');
nit: You can avoid nested quotes by shouldBeEqualToString.

https://codereview.chromium.org/58143002/diff/120001/Source/wtf/text/Base64.h
File Source/wtf/text/Base64.h (right):

https://codereview.chromium.org/58143002/diff/120001/Source/wtf/text/Base64.h...
Source/wtf/text/Base64.h:43: DoNotValidatePadding,
We should keep |Base64| prefix to decrease possibility of symbol conflict.

[Inactive, 2013-11-05 21:55:56]

https://codereview.chromium.org/58143002/diff/120001/LayoutTests/fast/dom/Win...
File LayoutTests/fast/dom/Window/atob-btoa.html (right):

https://codereview.chromium.org/58143002/diff/120001/LayoutTests/fast/dom/Win...
LayoutTests/fast/dom/Window/atob-btoa.html:42: shouldBe('window.atob(" YQ==")',
'"a"');
On 2013/11/05 21:39:07, tkent wrote:
> nit: You can avoid nested quotes by shouldBeEqualToString.

I know, I was too lazy to fix all of the pre-existing checks :)
Anyway, I will update all of them to use shouldBeEqualToString() before landing.

https://codereview.chromium.org/58143002/diff/120001/Source/wtf/text/Base64.h
File Source/wtf/text/Base64.h (right):

https://codereview.chromium.org/58143002/diff/120001/Source/wtf/text/Base64.h...
Source/wtf/text/Base64.h:43: DoNotValidatePadding,
On 2013/11/05 21:39:07, tkent wrote:
> We should keep |Base64| prefix to decrease possibility of symbol conflict.

Very good point, my bad.

[Inactive, 2013-11-05 22:14:06]

I fixed the nits, landing..

[commit-bot: I haz the power, 2013-11-05 22:16:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/58143002/200001

[commit-bot: I haz the power, 2013-11-05 23:34:24]

Change committed as 161395

[Inactive, 2013-11-11 14:21:12]

FYI, ap (Alexey) is pushing back on this change via
https://www.w3.org/Bugs/Public/show_bug.cgi?id=22731

I have a similar patch to get WebKit inline with us but it is currently blocked.