1033090 - crash in OOM | large | NS_ABORT_OOM(unsigned int) | AppendUTF8toUTF16(nsACString_internal const&, nsAString_internal&)

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is 
report bp-639cd958-5fa6-411b-97dd-85be22140625.
=============================================================

OOM Allocation Size	33556895

147 nsAutoString msg(NS_LITERAL_STRING("Image corrupt or truncated: ") +
148 NS_ConvertUTF8toUTF16(mImage.GetURIString()));

We should probably cap how much we copy from the URI here.
Not just for OOM, but also b/c megabyte long URIs in the
Console (and terminal window in debug builds) isn't useful.

Comment 1

[alex_mayorga]

My So's Firefox beta 32.0 crashed like this:

bp-66320294-016b-4f56-ba93-c55d62140801	01/08/2014	12:35 p.m.

Let me know if there's anything worth collecting from the crashing profile.

Comment 2

[alex_mayorga]

bp-47a9bd46-e924-4035-8332-de4a52140731	31/07/2014	01:09 p.m.

Comment 3

[alex_mayorga]

bp-9bde9688-0393-46a2-9af0-d5e0b2140731	31/07/2014	11:36 a.m.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mats Palmgren (:mats) from comment #0)
> This bug was filed from the Socorro interface and is 
> report bp-639cd958-5fa6-411b-97dd-85be22140625.
> =============================================================
> 
> OOM Allocation Size	33556895
> 
> 147 nsAutoString msg(NS_LITERAL_STRING("Image corrupt or truncated: ") +
> 148 NS_ConvertUTF8toUTF16(mImage.GetURIString()));
> 
> We should probably cap how much we copy from the URI here.
> Not just for OOM, but also b/c megabyte long URIs in the
> Console (and terminal window in debug builds) isn't useful.

And it actually shows up twice, as we pass both this msg as well as another NS_ConvertUTF8toUTF16(mImage.GetURIString()) to the call below.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Limit the size of URI returned on error?

Does going in this direction make sense?  If the URI is too large, show a portion of it, and let the user know that we're only showing a portion of it.  If the direction does make sense, what's a reasonable size to use? 1k, 50k? Does anybody ever look at more than the first few bytes to see if the header is mangled?

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8594074 [details] [diff] [review]
Limit the size of URI returned on error?

1k seems like enough to me.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8594074 [details] [diff] [review]
Limit the size of URI returned on error?

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

Thanks for tackling this, Milan!

::: image/src/Decoder.cpp
@@ +306,2 @@
>      if (consoleService && errorObject && !HasDecoderError()) {
> +      nsAutoString msg(NS_LITERAL_STRING("Image corrupt or truncated. "));

Nit: Move the space at the end of the string to the beginning of the optional second message. That way there's no extra space when it isn't needed.

@@ +311,5 @@
> +      const int32_t kOnlyShowThisMany = 1024;
> +      if (mImage->GetURIStringLength() <= kOnlyShowThisMany) {
> +        src = NS_ConvertUTF8toUTF16(mImage->GetURIString());
> +      } else {
> +        msg += NS_LITERAL_STRING("Showing URI truncated to 1024 bytes. ");

Just make this " (URI truncated due to length.)" or something similar. Then you don't need the comment on line 309.

@@ +318,2 @@
>  
>        if (NS_SUCCEEDED(errorObject->InitWithWindowID(

We should probably also do this truncation in ProgressTracker::FireFailureNotification.

::: image/src/ImageURL.h
@@ +37,5 @@
>    }
>  
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ImageURL)
>  
> +  nsresult GetSpec(nsACString& result, int32_t aGetFirstNCharOnly=-1)

I think we could make this a bit simpler if we added a new method instead of adding an optional parameter to GetSpec(). Just spitballin' here, but consider this alternative API: 

bool GetTruncatedSpec(nsACString& aResult);

This would write the possibly truncated spec into aResult, and then return |true| if it fit or |false| if it had to truncate. (You could also use an nsresult for the return type if you like XPCOM for some reason, or an enum class for bonus points.)

Then we don't need GetSpecLength() and we can encapsulate all the smarts in one place.

Also, it'd be nice not to make our implementation of GetSpec() diverge too much from nsIURI's behavior, since the long-term goal is to make nsIURI's threadsafe and get rid of ImageURL.

::: image/src/RasterImage.h
@@ +288,5 @@
>      }
>      return spec;
>    }
>  
> +  int GetURIStringLength() {

I'd suggest to just call Image::GetURI() in Decoder, which would give you an ImageURL object. Then you don't need to make any changes to this file.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Seth Fowler [:seth] from comment #7)
> ...
> 
> I think we could make this a bit simpler if we added a new method instead of
> adding an optional parameter to GetSpec(). Just spitballin' here, but
> consider this alternative API: 
> 
> bool GetTruncatedSpec(nsACString& aResult);
> 
> This would write the possibly truncated spec into aResult, and then return
> |true| if it fit or |false| if it had to truncate. (You could also use an
> nsresult for the return type if you like XPCOM for some reason, or an enum
> class for bonus points.)
> ...

I'll run with this idea; I was thinking we should pass in the maximum size into this method, but since we're only using it in one place, and we don't currently have another use, I will leave the magic encapsulated in that one function and worry about it if we need to in the future.  May actually build the max size into the function name.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Seth Fowler [:seth] from comment #7)
> ...
> 
> ::: image/src/Decoder.cpp
> @@ +306,2 @@
> >      if (consoleService && errorObject && !HasDecoderError()) {
> > +      nsAutoString msg(NS_LITERAL_STRING("Image corrupt or truncated. "));
> 
> Nit: Move the space at the end of the string to the beginning of the
> optional second message. That way there's no extra space when it isn't
> needed.

Actually, with or without the second message, we append the URI to it, so that space always ends up getting used.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #9)
> (In reply to Seth Fowler [:seth] from comment #7)
> > ...
> > 
> > ::: image/src/Decoder.cpp
> > @@ +306,2 @@
> > >      if (consoleService && errorObject && !HasDecoderError()) {
> > > +      nsAutoString msg(NS_LITERAL_STRING("Image corrupt or truncated. "));
> > 
> > Nit: Move the space at the end of the string to the beginning of the
> > optional second message. That way there's no extra space when it isn't
> > needed.
> 
> Actually, with or without the second message, we append the URI to it, so
> that space always ends up getting used.

Though we should stop doing that (adding the URI in two places :)

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Truncate large URIs before showing them to the user. r=seth

May have gone overboard with the enum and the method name.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8600411 [details] [diff] [review]
Truncate large URIs before showing them to the user. r=seth

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

Looks good!

BTW, I had mentioned in my previous comments that we should do this in ProgressTracker::FireFailureNotification as well, but actually that's a little bit trickier because the observers of net:failed-to-process-uri-content expect an nsIURI, not a string. We should probably just leave that one alone unless we get reported issues, because once we can get rid of ImageURL, no copy will occur in that method, and it'll get fixed for free. Which is all to say: this patch looks good as is.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Seth Fowler [:seth] from comment #12)
> ... Which is all to say: this patch looks good as is.

That's lucky, 'cause I was going to look at the extra piece you mentioned, and then lost track of it :)

Going for the paranoid try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a198d559128

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Truncate large URIs before showing them to the user. Carry r=seth

Rebased.  Also https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fb53a9eab54 before rebasing.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b30c80ba44f3

Comment 16

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/b30c80ba44f3

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows zero crashes in Firefox 41 over the past 4 weeks.

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=OOM+%7C+large+%7C+NS_ABORT_OOM%28unsigned+int%29+%7C+AppendUTF8toUTF16%28nsACString_internal+const%26%2C+nsAString_internal%26%29#tab-sigsummary