Bug 118898 – Make atob() throw an InvalidCharacterError on excess padding characters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

According to the latest specification, window.atob() should throw an
InvalidCharacterError on excess padding characters:
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#dom-windowbase64-atob (steps 4, 5 and 6)

This behavior is consistent with both Firefox, IE10 and since recently Blink.

Test suite:
http://www.w3c-test.org/html/tests/submission/AryehGregor/base64.html

Corresponding Blink revision:
https://src.chromium.org/viewvc/blink?revision=154538&view=revision

[Chris Dumez]

Created attachment 207081 [details]
Patch

[Benjamin Poulain]

Comment on attachment 207081 [details]
Patch

We don't need the pay the cost of this at runtime. You can have two functions, with and without padding, and make Base64PaddingValidationPolicy a template parameter.

[Chris Dumez]

Created attachment 207229 [details]
Patch

[Chris Dumez]

(In reply to comment #3)
> Created an attachment (id=207229) [details]
> Patch

Took Benjamin's feedback into consideration.

It is a bit unfortunate default function template parameters are only supported in C++11.

[Build Bot]

Comment on attachment 207229 [details]
Patch

Attachment 207229 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1177092

New failing tests:
fast/loader/user-stylesheet-fast-path.html

[Build Bot]

Created attachment 207234 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Chris Dumez]

Created attachment 207240 [details]
Patch

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 207081 [details])
> We don't need the pay the cost of this at runtime. You can have two functions, with and without padding, and make Base64PaddingValidationPolicy a template parameter.

I realized that padding validation only makes sense if we fail on invalid characters already (otherwise, there may be invalid characters / spaces at the end, after the padding or in the padding). Therefore, adding a new argument does not make much sense and it is better to add a new enumeration value such as "Base64FailOnInvalidCharacterOrExcessPadding".

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #2)
> > (From update of attachment 207081 [details] [details])
> > We don't need the pay the cost of this at runtime. You can have two functions, with and without padding, and make Base64PaddingValidationPolicy a template parameter.
> 
> I realized that padding validation only makes sense if we fail on invalid characters already (otherwise, there may be invalid characters / spaces at the end, after the padding or in the padding). Therefore, adding a new argument does not make much sense and it is better to add a new enumeration value such as "Base64FailOnInvalidCharacterOrExcessPadding".

Benjamin, could you please take another look?

[Build Bot]

Comment on attachment 207240 [details]
Patch

Attachment 207240 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1255430

New failing tests:
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref1.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/html/level2/events/dispatchEvent04.html
dom/html/level2/html/HTMLSelectElement20.html
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/html/level2/core/hc_namednodemapinvalidtype1.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
dom/html/level2/events/dispatchEvent01.html
dom/xhtml/level1/core/hc_attrappendchild4.xhtml
dom/html/level2/events/dispatchEvent03.html
dom/html/level2/events/dispatchEvent02.html
dom/html/level2/core/createDocumentType04.html
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
dom/html/level2/events/dispatchEvent06.html
css1/basic/comments.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi1.xhtml
dom/html/level2/events/dispatchEvent07.html
dom/html/level2/core/setAttributeNS10.html
dom/html/level2/events/dispatchEvent05.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level2/core/createAttributeNS06.html
dom/html/level1/core/hc_attrappendchild4.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreatepi.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref.xhtml
dom/xhtml/level1/core/hc_attrappendchild2.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html

[Build Bot]

Created attachment 207583 [details]
Archive of layout-test-results from APPLE-EWS-1 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-1  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Chris Dumez]

Any feedback on the latest patch?

[Chris Dumez]

Created attachment 208400 [details]
Patch

[Chris Dumez]

I updated the patch based on Benjamin's proposal on IRC.

[Chris Dumez]

Created attachment 208413 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 208413 [details]
Patch

Clearing flags on attachment: 208413

Committed r153904: <http://trac.webkit.org/changeset/153904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

*** Bug 104358 has been marked as a duplicate of this bug. ***