1133063 - base64 encoder/decoder should use fallible allocation

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mats Palmgren (inactive)]

Base64EncodeInputStream is used for base64-encoding blobs, canvas and is
used by nsScriptableBase64Encoder:
http://mxr.mozilla.org/mozilla-central/source/dom/workers/FileReaderSync.cpp#235
http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.cpp#503
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsScriptableBase64Encoder.cpp#20

I think such uses should use fallible allocation, so we should change
the SetLength calls in Base64.cpp to use the fallible version:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/Base64.cpp#183
It already has some error checking for OOM there so it seems that
fallible allocation was the original intention.

Also, I think these should actually return the result from the call:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsScriptableBase64Encoder.cpp#20

Comment 1

[Josh Matthews [:jdm]]

See https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation for an explanation of how to use fallible memory allocation.

Comment 2

[Titi_Alone]

Can I please work on this bug?

Comment 3

[Titi_Alone]

Attachment: Patch for bug 1133063

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8567506 [details] [diff] [review]
Patch for bug 1133063

No need for a superreview.  Those are not used much any more.

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 8567506 [details] [diff] [review]
Patch for bug 1133063

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

Looks good to me! Let's get the review of Nathan, who owns this code (see https://wiki.mozilla.org/Modules/Core).

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8567506 [details] [diff] [review]
Patch for bug 1133063

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

Looks good to me, with one minor change below.  Please upload a new patch with the change and flag me for review.

::: xpcom/io/Base64.cpp
@@ +184,1 @@
>    if (aDest.Length() != count + aOffset) {

Can you please move the SetLength call into the if's condition, like:

  if (!aDest.SetLength(count + aOffset, mozilla::fallible)) {
    ...
  }

Comment 7

[Titi_Alone]

Attachment: New patch for bug 1133063

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8570000 [details] [diff] [review]
New patch for bug 1133063

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

Thank you!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44495ed6c618

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/44495ed6c618