612642 - JS base64 code needs to be updated for removal of JS_GetStringBytesZ

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Robert Sayre]

I added some code that passes tests for purposes of merging, but there are still four functions here that are mostly copy/pasted. That turned into a maintenance pain immediately on our first encounter with this code.

I'll upload my patch to make sure it is kosher before it gets into mozilla-central.

Comment 1

[Robert Sayre]

Attachment: patch I landed to resolve the merge

Comment 2

[LegNeato]

This doesn't look to meet the standard of a beta8 blocker. Punting to betaN, feel free to disagree and/or triage to a different beta.

Comment 3

[Johnny Stenback (:jst)]

Ben, can you look over what sayrer did here and do any additional merging of code that makes sense to merge...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that this code leaks; see bug 616834.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Fixup, v1

This further consolidates the JS callers and reuses the quickstubs string conversion class, also fixes the leak.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 496222 [details] [diff] [review]
Fixup, v1

>diff --git a/dom/src/threads/nsDOMWorker.cpp b/dom/src/threads/nsDOMWorker.cpp
>--- a/dom/src/threads/nsDOMWorker.cpp
dom/src/threads/nsDOMWorker.cpp, nsDOMWorkerFunctions::AtoB:
>+  jsval result;
>+  if (!nsXPConnect::Base64Decode(aCx, JS_ARGV(aCx, aVp)[0], &result)) {
>     return JS_FALSE;
>   }
> 
>+  JS_SET_RVAL(aCx, aVp, result);
>   return JS_TRUE;

It's OK to write, in place of these 6 lines:

  return nsXPConnect::Base64Decode(aCx, JS_ARGV(aCx, aVp)[0], &JS_RVAL(aCx, aVp));

A little odd, that use of JS_RVAL, but OK.

Same thing in BtoA.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/tracemonkey/rev/f205194a4128

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #8)
> http://hg.mozilla.org/tracemonkey/rev/f205194a4128

Turned a weave crypt test orange... Investigating, backed out.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> (In reply to comment #8)
> > http://hg.mozilla.org/tracemonkey/rev/f205194a4128
> 
> Turned a weave crypt test orange... Investigating, backed out.

One of my initial patches to eliminate JS_GetStringBytes usage turned orange as I used strlen in Base64-related code. Since those strings can include \0 that was wrong. It looks like the patch is hit by this bug again as xpc_qsACString::xpc_qsACString uses strlen in xpcquickstubs.cpp. That strlen should be replaced with JS_GetStringEncodingLength(NULL, str) (where NULL is passed for cx as the errors already checked).

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > http://hg.mozilla.org/tracemonkey/rev/f205194a4128
> > 
> > Turned a weave crypt test orange... Investigating, backed out.
> 
> One of my initial patches to eliminate JS_GetStringBytes usage turned orange as
> I used strlen in Base64-related code. Since those strings can include \0 that
> was wrong. It looks like the patch is hit by this bug again as
> xpc_qsACString::xpc_qsACString uses strlen in xpcquickstubs.cpp. That strlen
> should be replaced with JS_GetStringEncodingLength(NULL, str) (where NULL is
> passed for cx as the errors already checked).

Even better solution would be to encode string directly into mBuf after resizing it. I.e. first call JS_GetStringEncodingLength(cx, str) to get the encoding length (but remember to check for an error) and then use HS_EncodeStringToBuffer.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/tracemonkey/rev/9a9e6e017929

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bah! I missed comments 10 and 11 this morning, sorry! I'll file a separate bug on avoiding the double copy.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Filed bug 618049.

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9a9e6e017929