Closed Bug 612642 Opened 9 years ago Closed 9 years ago

JS base64 code needs to be updated for removal of JS_GetStringBytesZ

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sayrer, Assigned: bent.mozilla)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
blocking2.0: --- → beta8+
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.
blocking2.0: beta8+ → betaN+
Ben, can you look over what sayrer did here and do any additional merging of code that makes sense to merge...
Component: XPConnect → DOM: Other
OS: Mac OS X → All
QA Contact: xpconnect → general
Hardware: x86 → All
No longer blocks: 616834
Depends on: 616834
Note that this code leaks; see bug 616834.
Duplicate of this bug: 616834
Attached patch Fixup, v1Splinter Review
This further consolidates the JS callers and reuses the quickstubs string conversion class, also fixes the leak.
Assignee: nobody → bent.mozilla
Attachment #490968 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #496222 - Flags: review?(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.
Attachment #496222 - Flags: review?(jorendorff) → review+
(In reply to comment #8)
> http://hg.mozilla.org/tracemonkey/rev/f205194a4128

Turned a weave crypt test orange... Investigating, backed out.
Whiteboard: fixed-in-tracemonkey
(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).
(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.
Bah! I missed comments 10 and 11 this morning, sorry! I'll file a separate bug on avoiding the double copy.
http://hg.mozilla.org/mozilla-central/rev/9a9e6e017929
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 618049
No longer depends on: 616834
Depends on: 616834
Component: DOM: Other → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.