901947 - don't null-check |new X| in content/

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

We have infallible new now.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: don't null-check |new X| in content/

Caught by grepping for NS_ERROR_OUT_OF_MEMORY and seeing if the checked thing
was allocated via |new|.

There are still a fair number of places where we throw NS_ERROR_OUT_OF_MEMORY.
Most of them are dealing with arrays, and I didn't want to check whether the
arrays in question were infallible or not.  Others are from various ::Create-type
functions and I wasn't sure those work identically to |new|.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 786289 [details] [diff] [review]
don't null-check |new X| in content/

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

::: content/html/content/src/HTMLFrameSetElement.cpp
@@ +239,5 @@
>      count++;
>      commaX = spec.FindChar(sComma, commaX + 1);
>    }
>  
>    nsFramesetSpec* specs = new nsFramesetSpec[count];

Should this be fallible?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 786289 [details] [diff] [review]
don't null-check |new X| in content/

I don't understand why we have (std::nothrow) with new. Remove it perhaps?

And yeah, could you make nsFramesetSpec* specs = new nsFramesetSpec[count]
use non-fallible new

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 0a - use fallible_t instead of std::nothrow in CanvasRenderingContext2D.cpp

As requested.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 0b - use fallible allocation in HTMLFrameSetElement.cpp

As requested.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 1 - don't null-check |new X| in content/

Carrying over r+.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 787673 [details] [diff] [review]
part 0a - use fallible_t instead of std::nothrow in CanvasRenderingContext2D.cpp

># HG changeset patch
># User Nathan Froyd <[e-mail]>
>
>part 0a - use fallible_t instead of std::nothrow in CanvasRenderingContext2D.cpp
>
>As requested.
>
>diff --git a/content/canvas/src/CanvasRenderingContext2D.cpp b/content/canvas/src/CanvasRenderingContext2D.cpp
>index fe50689..8fe6c63 100644
>--- a/content/canvas/src/CanvasRenderingContext2D.cpp
>+++ b/content/canvas/src/CanvasRenderingContext2D.cpp
>@@ -1036,31 +1036,32 @@ CanvasRenderingContext2D::GetInputStream(const char *aMimeType,
>   nsRefPtr<gfxASurface> surface;
> 
>   if (NS_FAILED(GetThebesSurface(getter_AddRefs(surface)))) {
>     return NS_ERROR_FAILURE;
>   }
> 
>   nsresult rv;
>   const char encoderPrefix[] = "@mozilla.org/image/encoder;2?type=";
>-  nsAutoArrayPtr<char> conid(new (std::nothrow) char[strlen(encoderPrefix) + strlen(aMimeType) + 1]);
>+  static const fallible_t fallible = fallible_t();
>+  nsAutoArrayPtr<char> conid(new (fallible) char[strlen(encoderPrefix) + strlen(aMimeType) + 1]);
> 
>   if (!conid) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
> 
>   strcpy(conid, encoderPrefix);
>   strcat(conid, aMimeType);
> 
>   nsCOMPtr<imgIEncoder> encoder = do_CreateInstance(conid);
>   if (!encoder) {
>     return NS_ERROR_FAILURE;
>   }
> 
>-  nsAutoArrayPtr<uint8_t> imageBuffer(new (std::nothrow) uint8_t[mWidth * mHeight * 4]);
>+  nsAutoArrayPtr<uint8_t> imageBuffer(new (fallible) uint8_t[mWidth * mHeight * 4]);
>   if (!imageBuffer) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
> 
>   nsRefPtr<gfxImageSurface> imgsurf =
>     new gfxImageSurface(imageBuffer.get(),
>                         gfxIntSize(mWidth, mHeight),
>                         mWidth * 4,

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 787674 [details] [diff] [review]
part 0b - use fallible allocation in HTMLFrameSetElement.cpp

>@@ -243,17 +245,18 @@ HTMLFrameSetElement::ParseRowCol(const nsAString & aValue,
>   PR_STATIC_ASSERT(NS_MAX_FRAMESET_SPEC_COUNT * sizeof(nsFramesetSpec) < (1 << 30));
>   int32_t commaX = spec.FindChar(sComma);
>   int32_t count = 1;
>   while (commaX != kNotFound && count < NS_MAX_FRAMESET_SPEC_COUNT) {
>     count++;
>     commaX = spec.FindChar(sComma, commaX + 1);
>   }
> 
>-  nsFramesetSpec* specs = new nsFramesetSpec[count];
>+  static const fallible_t fallible = fallible_t();
>+  nsFramesetSpec* specs = new (fallible) nsFramesetSpec[count];
I think we need only this case, since web page can control the 'count'

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c496a6a8d2f
https://hg.mozilla.org/mozilla-central/rev/63308a3e87ad
https://hg.mozilla.org/mozilla-central/rev/a3ea5bc816f5