:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 13 years ago

See <http://html5.org/tools/web-apps-tracker?from=6873&to=6874>.

Code is at <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#2876>.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 13 years ago

This requires either changing NSPR behavior (wtc, would that be acceptable?) or reimplementing base64 stuff outside NSPR.

	Wan-Teh Chang
	Comment 2 • 13 years ago

Changing PL_Base64Decode behavior should be OK.  The change is a little
tricky because the 'srclen' input argument may be an over-estimate.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 3 • 13 years ago

Note that we already have base64 code outside of nspr (xpcom/io/Base64.cpp).

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 4 • 13 years ago

Also, is this really supposed to remove internal space characters?  What's the point of this?

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 13 years ago

Base64 content, often has spaces inserted to break it up into sane-length lines, especially when going over SMTP.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 6 • 13 years ago

Oh, atob, not btoa.  Ignore me :-)

	Wan-Teh Chang
	Comment 7 • 13 years ago

I found that the base64 decoder in NSS already ignores whitespace:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/nssb64d.c&rev=1.8&mark=265-279#265

NSS uses its own base64 decoder (rather than PL_Base64Decode) to
decode PEM certificate files, which contain newlines.

	Tim
	Comment 8 • 13 years ago

I would like to tackle this bug.  How do I get attached to it?

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 13 years ago

I just assigned it to you.  Past that, just post patches.  And thank you!

	Mike Hoye [:mhoye]
	Comment 10 • 11 years ago

Can you confirm that you're still working on this bug?

	Michael Assignee
	Comment 11 • 11 years ago

I'd like to work on this one, please.

	Mike Hoye [:mhoye]
	Comment 12 • 11 years ago

OK, I've assigned it to you, and it's all yours. Good luck, and please let us know about your progress, or any questions you have, in this bug.

Happy hunting!

	Michael Assignee
	Comment 13 • 11 years ago

Should other characters be ignored as well, or just whitespace?

	Michael Assignee
	Comment 14 • 11 years ago

Oh, and we can say the input to atob must have a even number of characters, correct?

	Michael Assignee
	Comment 15 • 11 years ago

Anyhow, waiting for mach to build me a test.

	Masatoshi Kimura [:emk]
	Comment 16 • 11 years ago

Please see the spec about the exact parsing behavior.
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#dom-windowbase64-atob

	Michael Assignee
	Comment 17 • 11 years ago

OK, thanks!

	Michael Assignee
	Comment 18 • 11 years ago

OK, got that; currently waiting for a test build. If it works I'll attach a patch.

	Michael Assignee
	Comment 19 • 11 years ago

Almost done with this.

	mjh563
	Comment 21 • 11 years ago

Michael, it would be better to request a review from a specific person. I'm not sure who'd be appropriate though. Maybe :Ms2ger since they're the bug mentor?

	Michael Assignee
	Comment 22 • 11 years ago

OK, trying that.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 23 • 11 years ago

Comment on attachment 779212 [details] [diff] [review]
Proposed patch v1

For NSPR changes, you'll want wtc.

	Wan-Teh Chang
	Comment 25 • 11 years ago

Comment on attachment 780030 [details] [diff] [review]
Proposed patch v1.1

Review of attachment 780030 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch. I only reviewed nsprpub/lib/libc/src/base64.c.
I didn't review it in detail because I spotted inappropriate use of
global variables.

::: nsprpub/lib/libc/src/base64.c
@@ +10,5 @@
>  #include <string.h> /* for strlen */
>  
>  static unsigned char *base = (unsigned char *)"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
> +static PRUint32 lenm = (PRUint32)0;
> +static PRUint32 dcn = (PRUint32)0;

lenm and dcn are global variables, but the functions modify them.
So two threads cannot call the functions at the same time. We need
a different approach so that the functions remain thread-safe.

Also the variable names "lenm" and "dcn" are not descriptive enough.
I can't guess what they mean.

@@ +184,5 @@
> +        return -2;
> +    }
> +    else if( (unsigned char)' ' == c || (unsigned char)'\r' == c ||
> +        (unsigned char)'\n' == c || (unsigned char)'\t' == c ||
> +        (unsigned char)'\f' == c )

Use the isspace(c) function from <ctype.h>.

@@ +446,5 @@
>          }
> +        for( i = 0; i < strlen(src); i++ ) {
> +            if ( (unsigned char)' ' == src[i] || (unsigned char)'\r' == src[i] ||
> +            (unsigned char)'\n' == src[i] || (unsigned char)'\t' == src[i] ||
> +            (unsigned char)'\f' == src[i] ) {

Use isspace((unsigned char)src[i]) here.

	Masatoshi Kimura [:emk]
	Comment 26 • 11 years ago

(In reply to Wan-Teh Chang from comment #25)
> @@ +184,5 @@
> > +        return -2;
> > +    }
> > +    else if( (unsigned char)' ' == c || (unsigned char)'\r' == c ||
> > +        (unsigned char)'\n' == c || (unsigned char)'\t' == c ||
> > +        (unsigned char)'\f' == c )
> 
> Use the isspace(c) function from <ctype.h>.

The definition of "white space" is included in the HTML5 spec and we can't assume that isspace(c) matches the definition.

I think we should use xpcom/io/Base64.cpp or something instead of bringing HTML5'ism into NSPR.

	Masatoshi Kimura [:emk]
	Comment 28 • 11 years ago

Comment on attachment 782855 [details] [diff] [review]
Proposed patch v2

Did you read comment #26?
This patch will violate the HTML spec.
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#space-character
Even the current locale will affect the isspace() result. isspace() is not adequate here at all.
But the NSPR owner refused the first patch, so we will have to add or use a base64 decoder outside NSPR.

	Michael Assignee
	Comment 29 • 11 years ago

(In reply to Masatoshi Kimura [:emk] from comment #28)
> Comment on attachment 782855 [details] [diff] [review]
> Proposed patch v2
> 
> Did you read comment #26?
> This patch will violate the HTML spec.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/common-
> microsyntaxes.html#space-character
> Even the current locale will affect the isspace() result. isspace() is not
> adequate here at all.
> But the NSPR owner refused the first patch, so we will have to add or use a
> base64 decoder outside NSPR.

OK

	Masatoshi Kimura [:emk]
	Comment 33 • 11 years ago

Comment on attachment 785509 [details] [diff] [review]
Proposed patch v3.2

>+  in.SetCapacity(aAsciiBase64String.Length());

I have a concern about wasting memory by copying the string. At least a fallible allocator should be used because Web pages can pass arbitrary large string.

>+    if(!(nsCRT::IsAsciiSpace(*(start)) || *(start) == '\f')) {

Please use nsContentUtils::IsHTMLWhitespace.

	Michael Assignee
	Comment 34 • 11 years ago

(In reply to Masatoshi Kimura [:emk] from comment #33)
> Comment on attachment 785509 [details] [diff] [review]
> Proposed patch v3.2
> 
> >+  in.SetCapacity(aAsciiBase64String.Length());
> 
> I have a concern about wasting memory by copying the string. At least a
> fallible allocator should be used because Web pages can pass arbitrary large
> string.
> 
> >+    if(!(nsCRT::IsAsciiSpace(*(start)) || *(start) == '\f')) {
> 
> Please use nsContentUtils::IsHTMLWhitespace.

Addressing now.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 36 • 11 years ago

Comment on attachment 788712 [details] [diff] [review]
Proposed patch v4

Review of attachment 788712 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

I wonder if this is too much work for the case where there are no spaces in the input. Maybe you could first do a loop over the input to check if the new string is necessary. (Possibly merged with the Is8Bit call somehow... That's probably a premature optimization, though.)

::: content/base/src/nsContentUtils.cpp
@@ +652,5 @@
>      aBinaryData.Truncate();
>      return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
>    }
>  
> +  const PRUnichar *start = aAsciiBase64String.BeginReading();

* to the left

@@ +653,5 @@
>      return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
>    }
>  
> +  const PRUnichar *start = aAsciiBase64String.BeginReading();
> +  const PRUnichar *end = aAsciiBase64String.EndReading();

Same here

@@ +654,5 @@
>    }
>  
> +  const PRUnichar *start = aAsciiBase64String.BeginReading();
> +  const PRUnichar *end = aAsciiBase64String.EndReading();
> +  nsString in;

Maybe 'trimmedBase64String' or something like that?

@@ +655,5 @@
>  
> +  const PRUnichar *start = aAsciiBase64String.BeginReading();
> +  const PRUnichar *end = aAsciiBase64String.EndReading();
> +  nsString in;
> +  if(!in.SetCapacity(aAsciiBase64String.Length(), fallible_t())) {

'if ('

@@ +658,5 @@
> +  nsString in;
> +  if(!in.SetCapacity(aAsciiBase64String.Length(), fallible_t())) {
> +    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
> +  }
> +  while (start != end) {

Let's use < just to be sure

@@ +659,5 @@
> +  if(!in.SetCapacity(aAsciiBase64String.Length(), fallible_t())) {
> +    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
> +  }
> +  while (start != end) {
> +    if(!nsContentUtils::IsHTMLWhitespace(*start)) {

'if ('

@@ +660,5 @@
> +    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
> +  }
> +  while (start != end) {
> +    if(!nsContentUtils::IsHTMLWhitespace(*start)) {
> +      in.Append(start, 1);

Append(*start) should work, I think

@@ +664,5 @@
> +      in.Append(start, 1);
> +    }
> +    start++;
> +  }
> +  nsresult rv = Base64Decode((const nsAString&)in, aBinaryData);

Should not need this cast

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 37 • 11 years ago

And we need a mochitest.

	Michael Assignee
	Comment 38 • 11 years ago

I'll do it tomorrow. Where does the mochitest go in the tree?

	Michael Assignee
	Comment 39 • 11 years ago

Ignore the above.

	Michael Assignee
	Comment 40 • 11 years ago

Patch with review comments addressed.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 42 • 11 years ago

Comment on attachment 801270 [details] [diff] [review]
Mochitest

Review of attachment 801270 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: content/base/test/test_bug711180.html
@@ +11,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=711180">Mozilla Bug 711180</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

Remove the trailing whitespace here.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 43 • 11 years ago

Comment on attachment 798143 [details] [diff] [review]
Proposed patch v5

Review of attachment 798143 [details] [diff] [review]:
-----------------------------------------------------------------

> Atob should ignore whitespace - 711180

Make that "Bug 711180 - Ignore whitespace in atob; r=Ms2ger"

and put the test in this patch too.

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 11 years ago

Comment on attachment 802658 [details] [diff] [review]
Patch for checkin; r=Ms2ger

Thanks for the patch, Michael! In the future, you can just set the checkin-needed bug keyword instead, though. It's a bit easier for us to work with.

	Ryan VanderMeulen [:RyanVM]
	Comment 46 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5d584bc36c

	Phil Ringnalda (:philor)
	Comment 47 • 11 years ago

And backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/315cc4fb111a - turns out we have some expected failures in http://mxr.mozilla.org/mozilla-central/source/dom/imptests/failures/html/html/webappapis/atob/test_base64.html.json?force=1 which no longer fail with this patch and need to have their annoatations adjusted, see https://tbpl.mozilla.org/php/getParsedLog.php?id=27669092&tree=Mozilla-Inbound

	Michael Assignee
	Comment 48 • 11 years ago

(In reply to Phil Ringnalda (:philor) from comment #47)
> And backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/315cc4fb111a - turns
> out we have some expected failures in
> http://mxr.mozilla.org/mozilla-central/source/dom/imptests/failures/html/
> html/webappapis/atob/test_base64.html.json?force=1 which no longer fail with
> this patch and need to have their annoatations adjusted, see
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27669092&tree=Mozilla-
> Inbound

Should those just be removed, now that they are capable of passing?

	Phil Ringnalda (:philor)
	Comment 49 • 11 years ago

I presume so, but the reason I backed it out rather than fixing it was mostly because I don't actually know how either those annotations or the entire testsuite works.

	neil@parkwaycc.co.uk
	Comment 50 • 11 years ago

Comment on attachment 802658 [details] [diff] [review]
Patch for checkin; r=Ms2ger

>+  bool needTrimming = false;
>+  while (iter < end) {
>+    if (!nsContentUtils::IsHTMLWhitespace(*iter)) {
>+      if (!needTrimming) {
>+        needTrimming = true;
>+        trimmedString.Append(start, iter - start);
>+      }
>+      trimmedString.Append(*iter);
>+    }
>+    iter++;
>+  }
OK, so the bit I understand is this section:
while (iter < end) {
  if (!nsContentUtils::IsHTMLWhitespace(*iter))
    trimmedString.Append(*iter);
  iter++;
}
What I can't figure out is what, if anything, the needTrimming code gets you. My best guess is that perhaps you wanted the result string to be the original string, but if you find some whitespace then you make a copy of the string so far and start appending non-whitespace to it.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 51 • 11 years ago

Where does the needTrimming code come from? I didn't see that in the patch I reviewed...

For the test, you should be able to remove the lines in

http://mxr.mozilla.org/mozilla-central/source/dom/imptests/failures/html/html/webappapis/atob/test_base64.html.json?force=1

that test whitespace. Looks like that's 3--12.

	Michael Assignee
	Comment 52 • 11 years ago

OOPS. It helps if I upload the right one. OK, thanks!

	Michael Assignee
	Comment 56 • 11 years ago

INI changes

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 58 • 11 years ago

Comment on attachment 814256 [details] [diff] [review]
Removed duplicate INI entry

Review of attachment 814256 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

	Ed Morley [:emorley]
	Comment 60 • 11 years ago

Pushed this along with a bunch of other checkin-neededs to try:
https://tbpl.mozilla.org/?tree=Try&rev=503271ae46d1

	Ed Morley [:emorley]
	Comment 61 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a151e014c562

	Carsten Book [:Tomcat]
	Comment 62 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/a151e014c562

	Kohei Yoshino [:kohei]
	Comment 63 • 11 years ago

https://developer.mozilla.org/en-US/docs/Web/API/Window.atob
https://developer.mozilla.org/en-US/Firefox/Releases/27/Site_Compatibility