326854 - expose btoa and atob to JS components

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[:Gavin Sharp [email: [e-mail]]]

I'm trying to base64 encode a string from a JS component (for creating data: URIs for search engine icons) and would love to be able to use the handy atob/btoa functions that are available to content JS via nsGlobalWindow::Btoa/nsGlobalWindow::Atob. Shaver mentioned that it would be possible to expose these functions (PL_Base64Encode/PL_Base64Decode, essentially) to JS components in the global scope much like "dump" and "debug" are.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

I too would like to use these functions, in my case to create data: URLs in the microsummary service per the proposed fix for bug 339543.

It looks like safe browsing also does base64 en/decoding via the 
toolkit/components/url-classifier/content/moz/base64.js and extensions/safe-browsing/lib/moz/base64.js scripts, although these apparently need a custom "websafe base64" encoding function and thus are probably not candidates for refactoring.

Comment 2

[Asaf Romano (gone)]

Attachment: v1

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 248315 [details] [diff] [review]
v1

>Index: js/src/xpconnect/loader/mozJSComponentLoader.cpp

>+#include "jsstr.h"

I think this wants to be jsapi.h, but I defer to sager SpiderMonkey gurus.


>+    PRInt32 dataLen = JSSTRING_LENGTH(str);

Must this be a signed value?  It's a length, after all.  :-)


>+    char *base64 = JS_GetStringBytes(str);

Missing a null-check of base64 here if JS_GetStringBytes fails.


>+    resultLen = ((resultLen * 3) / 4);

Maybe I'm too paranoid, but if resultLen is big you could have an overflow here.  I think / 4 and then * 3 has the same semantics and can't cause problems.


>+    if (!bin_data) {
>+        nsMemory::Free(base64);
>+        return JS_FALSE;
>+    }

This is wrong; base64 is owned by the JS GC mechanism and should not be manually freed: <http://developer.mozilla.org/en/docs/JS_GetStringBytes>.


>+    PRInt32 resultLen = ((JSSTRING_LENGTH(str) + 2) / 3) * 4;

Again, signedness and overflow paranoia here.  ;-)


>+    if (!base64) {
>+        nsMemory::Free(bin_data);
>+        return JS_FALSE;
>+    }

Again the erroneous free.


Also, the PL_Base64* functions probably need to be PR_freed or something; I didn't check the docs, but I think what you currently have leaks.

Comment 4

[Asaf Romano (gone)]

Attachment: v1.1

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 248319 [details] [diff] [review]
v1.1

>+#include "jsstr.h"

No need for this if you use JS_GetStringLength(str) here:

>+    PRInt32 dataLen = JSSTRING_LENGTH(str);
>+    char *base64 = JS_GetStringBytes(str);

Without IDL and XPConnect helping, someone could pass in a string with high byte bits set in one or more chars -- do you care? You'll throw 'em away silently. IIRC for nsIDOMWindowInternal.btoa, XPConnect will complain in a DEBUG build and truncate in a release build. Not a big deal, thought I'd ask.

>+    char *bin_data = PL_Base64Decode(base64, dataLen,
>+                                     nsnull);

Doesn't the call fit on one line?  Nit-picking only, but the line would wrap without the single-use base64 and dataLen temporaries.  Could get rid of those at the price of more complicated call expression, or at least rationalize names by using "data" with "dataLen", or some such pairing (base64/base64len, dataPtr/dataLen, etc.).

>+    if (!bin_data)
>+        return JS_FALSE;
>+
>+    JSString *rvalString = JS_NewString(cx, bin_data, resultLen);

Is PL_Base64Decode guaranteed to use malloc, the same malloc whose free the JS GC will call when this new string is finalized?

Suggest reusing str here instead of rvalString.

>+    char *bin_data = JS_GetStringBytes(str);
>+    if (!bin_data)
>+        return JS_FALSE;

JS_GetStringBytes never returns null, even for OOM (it returns "", but you can't distinguish that as OOM without testing JS_GetStringLength(str), and it's not generally worth doing that).

>+    PRInt32 resultLen = ((JSSTRING_LENGTH(str) + 2) / 3) * 4;
>+    char *base64 = PL_Base64Encode(bin_data, JSSTRING_LENGTH(str), nsnull);

(/me notes lack of single-use dataLen variable here ;-)

Again the "which malloc" question.  Windows and other systems that support different mallocs per DLL/DSO might require you here and earlier to do your own JS_malloc calls, and pass non-null pointers as the final arguments to PL_Base64{De,En}code.

/be

Comment 6

[Asaf Romano (gone)]

Attachment: v1.2

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (From update of attachment 248319 [details] [diff] [review] [edit])
>+    PRInt32 dataLen = JSSTRING_LENGTH(str);

size_t as jwalden noted.

/be

Comment 8

[Asaf Romano (gone)]

Comment on attachment 248322 [details] [diff] [review]
v1.2

didn't notice brendan commented meanwhile.

Comment 9

[Asaf Romano (gone)]

Attachment: v1.3

Comment 10

[Brendan Eich [:brendan]]

I hope timeless or bsmedberg will remind us about the DLL-specific malloc/free issue and whether it could bite XPConnect vs. SpiderMonkey (see comment 5).

/be

Comment 11

[Asaf Romano (gone)]

(In reply to comment #3)

> 
> >+    resultLen = ((resultLen * 3) / 4);
> 
> Maybe I'm too paranoid, but if resultLen is big you could have an overflow
> here.  I think / 4 and then * 3 has the same semantics and can't cause
> problems.


I left |((JSSTRING_LENGTH(str) + 2) / 3) * 4| as is to keep this in sync with the nsGlobalWindow implementation.


> 
> >+    PRInt32 resultLen = ((JSSTRING_LENGTH(str) + 2) / 3) * 4;
> 
> Again, signedness and overflow paranoia here.  ;-)

ditto.


(In reply to comment #5)

> Without IDL and XPConnect helping, someone could pass in a string with high
> byte bits set in one or more chars -- do you care? You'll throw 'em away
> silently. IIRC for nsIDOMWindowInternal.btoa, XPConnect will complain in a
> DEBUG build and truncate in a release build. Not a big deal, thought I'd ask.

Since these are only available for JS components, I'm not sure we should sanity-check the strings.

> >+    char *bin_data = PL_Base64Decode(base64, dataLen,
> >+                                     nsnull);
> 
> Doesn't the call fit on one line?  Nit-picking only, but the line would wrap
> without the single-use base64 and dataLen temporaries.  Could get rid of those
> at the price of more complicated call expression, or at least rationalize names
> by using "data" with "dataLen", or some such pairing (base64/base64len,
> dataPtr/dataLen, etc.).

fixed.

> 
> >+    if (!bin_data)
> >+        return JS_FALSE;
> >+
> >+    JSString *rvalString = JS_NewString(cx, bin_data, resultLen);
> 
> Is PL_Base64Decode guaranteed to use malloc, the same malloc whose free the JS
> GC will call when this new string is finalized?

avoided this assumption by using JS_NewStringCopyN.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 248330 [details] [diff] [review]
v1.3

>+    size_t base64StrLength = JS_GetStringLength(str);
>+    char *base64Str = JS_GetStringBytes(str);
>+
>+    PRUint32 bin_dataLength = base64StrLength;
>+    if (base64Str[base64StrLength - 1] == '=') {
>+        if (base64Str[base64StrLength - 2] == '=')
>+            bin_dataLength = base64StrLength - 2;
>+        else
>+            bin_dataLength = base64StrLength - 1;
>+    }
>+    bin_dataLength = (bin_dataLength * 3) / 4;

Two points:

1.  It's more idiomatic and concise to write

>+    PRUint32 bin_dataLength = base64StrLength;
>+    if (base64Str[base64StrLength - 1] == '=') {
>+        if (base64Str[base64StrLength - 2] == '=')
>+            bin_dataLength -= 2;
>+        else
>+            --bin_dataLength;
>+    }

It might yield smaller code, without a noticeable performance hit, to write

>+    PRUint32 bin_dataLength = base64StrLength;
>+    if (base64Str[base64StrLength - 1] == '=') {
>+        --bin_dataLength;
>+        if (base64Str[base64StrLength - 2] == '=')
>+            --bin_dataLength;
>+    }

2.  base64StrLength is size_t but bin_dataLength is PRUint32, so 64-bit systems will probably get a warning about the truncation from 64 to 32 bits.  This argues for the above change to avoid reassigning, minimizing the (PRUint32) cast to one place: the initialization of bin_dataLength.

You've avoided malloc/free mismatch hassles by always copying, and the code's simpler for it. I'm ok with it. If we ever found a hot spot due to the separate malloc'd copy, we could avoid it as proposed in an earlier comment by allocating and passing the non-null buffer pointer into the PL_Base64*code functions.

Fix the above idiom/cast issue and r=me.

/be

Comment 13

[Asaf Romano (gone)]

Attachment: patch

is required for xpconnect/?

Comment 14

[Asaf Romano (gone)]

s/is/is sr/

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 248334 [details] [diff] [review]
patch


>+    bin_dataLength = (bin_dataLength * 3) / 4;

Make this 'bin_dataLength = (PRUint32)((PRUint64)bin_dataLength * 3) / 4);' to allay fears of wraparound in 32 bits on the multiply, and we can all get on with our lives ;-).

r=me with that. Please attach the patch you land for posterity/branch-merging.

/be

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 248334 [details] [diff] [review]
patch

Nit of the day:

 static JSFunctionSpec gGlobalFun[] = {
     {"dump",    Dump,   1,0,0},
     {"debug",   Debug,  1,0,0},
+    {"atob",    Atob,   1,0,0},
+    {"btoa",    Btoa,   1,0,0},
+
     {nsnull,nsnull,0,0,0}

No need for the empty line there :)

sr=jst

Comment 17

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp 1.130

Comment 18

[Eric Shepherd [:sheppy]]

Are there any examples available of this in use?  I presume this is only for use in JS components and otherwise isn't available for use elsewhere.

Comment 19

[Jeff Walden [:Waldo]]

For some reason they're already available to web pages:

http://developer.mozilla.org/en/docs/DOM:window.atob
http://developer.mozilla.org/en/docs/DOM:window.btoa

A strategically-placed link would probably suffice, although I have no idea where the best place for such a link is, beyond the "New in..." page.

Comment 20

[Eric Shepherd [:sheppy]]

OK, these are now mentioned on Fx3 for developers, with links to the DOM functions of the same names.  Eventually these will be documented properly when we finally get XPCOM stuff documented.

Comment 21

[Nickolay_Ponomarev]

The fact that atob/btoa are available from JS components could also be mentioned on the DOM reference pages, like we did with http://developer.mozilla.org/en/docs/DOM:window.dump