expose btoa and atob to JS components

RESOLVED FIXED in mozilla1.9alpha2

Status

()

enhancement
P3
normal
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: Gavin, Assigned: mano)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9alpha2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 5 obsolete attachments)

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.
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.

Assignee: dbradley → nobody
QA Contact: pschwartau → xpconnect
Posted patch v1 (obsolete) — Splinter Review
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #248315 - Flags: review?
Target Milestone: mozilla1.8.1 → ---
Attachment #248315 - Flags: review? → review?(brendan)
Priority: -- → P3
Version: 1.8 Branch → Trunk
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.
Attachment #248315 - Flags: review?(brendan) → review-
Posted patch v1.1 (obsolete) — Splinter Review
Attachment #248315 - Attachment is obsolete: true
Attachment #248319 - Flags: review?(brendan)
No longer blocks: 317107
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
Posted patch v1.2 (obsolete) — Splinter Review
Attachment #248319 - Attachment is obsolete: true
Attachment #248322 - Flags: review?(brendan)
Attachment #248319 - Flags: review?(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 on attachment 248322 [details] [diff] [review]
v1.2

didn't notice brendan commented meanwhile.
Attachment #248322 - Flags: review?(brendan)
Posted patch v1.3 (obsolete) — Splinter Review
Attachment #248322 - Attachment is obsolete: true
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
(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 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
Posted patch patch (obsolete) — Splinter Review
is required for xpconnect/?
Attachment #248330 - Attachment is obsolete: true
Attachment #248334 - Flags: review?(brendan)
Attachment #248330 - Flags: review?(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
Attachment #248334 - Flags: review?(brendan) → review+
Attachment #248334 - Flags: superreview?(shaver) → superreview?(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
Attachment #248334 - Flags: superreview?(jst) → superreview+
Posted patch as checked inSplinter Review
mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp 1.130
Attachment #248334 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Flags: in-testsuite?
Keywords: dev-doc-needed
Whiteboard: [19a2]
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha2
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.
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.
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.
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

You need to log in before you can comment on or make changes to this bug.