ImageOps::DecodeToSurface tries to complete an already complete Sourcebuffer

RESOLVED FIXED in Firefox 54

Status

()

Core
ImageLib
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 months ago
Causes an assertion similar to bug 1238574.
Comment hidden (mozreview-request)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8834967 [details]
Bug 1337829 - ImageOps::DecodeToSurface tries to complete an already complete Sourcebuffer.

https://reviewboard.mozilla.org/r/110704/#review112054

::: image/ImageOps.cpp:119
(Diff revision 1)
>      return nullptr;
>    }
> +  // Make sure our sourceBuffer is marked as complete.
> +  if (!sourceBuffer->IsComplete()) {
> -  sourceBuffer->Complete(NS_OK);
> +    sourceBuffer->Complete(NS_OK);
> +  }

SourceBuffer can only be completed if either Complete or HandleError is called. Are you hitting an OOM condition? Is that to be expected for the use case?
Attachment #8834967 - Flags: review?(aosmond) → review+
(Assignee)

Comment 3

7 months ago
mozreview-review-reply
Comment on attachment 8834967 [details]
Bug 1337829 - ImageOps::DecodeToSurface tries to complete an already complete Sourcebuffer.

https://reviewboard.mozilla.org/r/110704/#review112054

> SourceBuffer can only be completed if either Complete or HandleError is called. Are you hitting an OOM condition? Is that to be expected for the use case?

I don't think it was an OOM condition, I am decoding tiny images (favicons, so nothing bigger than 256x256) and I'm on 64bit.
Maybe it was an error, I'm not sure yet, I will check again as soon as I can actually test my build.
(Assignee)

Updated

7 months ago
Assignee: nobody → mak77

Comment 4

7 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/f3b02e3b5207
ImageOps::DecodeToSurface tries to complete an already complete Sourcebuffer.r=aosmond

Comment 5

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3b02e3b5207
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Marco Bonardo [::mak] from comment #3)
> I don't think it was an OOM condition, I am decoding tiny images (favicons,
> so nothing bigger than 256x256) and I'm on 64bit.
> Maybe it was an error, I'm not sure yet, I will check again as soon as I can
> actually test my build.

Yes, please do. We'd like to know why this invariant isn't holding.
Flags: needinfo?(mak77)
(Assignee)

Comment 7

6 months ago
Ok, you were right, this is the stack settings the Complete status:

>	xul.dll!mozilla::image::SourceBuffer::HandleError(nsresult aError) Line 658	C++
 	xul.dll!mozilla::image::SourceBuffer::ExpectLength(unsigned int aExpectedLength) Line 321	C++
 	xul.dll!mozilla::image::ImageOps::DecodeToSurface(nsIInputStream * aInputStream, const nsACString_internal & aMimeType, unsigned int aFlags) Line 112	C++
 	xul.dll!mozilla::places::FetchAndConvertUnsupportedPayloads::ConvertPayload(__int64 aId, const nsACString_internal & aMimeType, nsCString & aPayload, int * aWidth) Line 1019	C++

Looks like this fails:
if (MOZ_UNLIKELY(NS_FAILED(AppendChunk(CreateChunk(aExpectedLength))))) {
    return HandleError(NS_ERROR_OUT_OF_MEMORY);
  }

aExpectedLength is 2550
And I see this warning:
"SourceBuffer refused to create chunk too large for SurfaceCache"

The strange thing is that in a later version of my patch, this error disappears. It's possible I changed the icons that I'm trying to convert (I could check this).
Looks like the limit is too small if it can't create a chunk for a simple ico file?
Flags: needinfo?(mak77) → needinfo?(tnikkel)
Thanks for looking into that.

2550 should not be too large to fit in the surface cache. Something must be going wrong in the calculation of the size of the surface cache.

The CanHold call would be returning false when comparing against mMaxCost

https://dxr.mozilla.org/mozilla-central/rev/32dcdde1fc64fc39a9065dc4218265dbc727673f/image/SurfaceCache.cpp#651

The value of mMaxCost is computed here:

https://dxr.mozilla.org/mozilla-central/rev/32dcdde1fc64fc39a9065dc4218265dbc727673f/image/SurfaceCache.cpp#986

It is based on the value of the call PR_GetPhysicalMemorySize() and the two preferences image.mem.surfacecache.max_size_kb and image.mem.surfacecache.size_factor. I guess one of those three values must be out of whack?
Flags: needinfo?(tnikkel) → needinfo?(mak77)
(On a modern laptop I'd expect the size of the surface cache to be around 1 GB, so 2550 bytes is way small.)
(Assignee)

Comment 10

6 months ago
OK, I think I now have a final answer to all of this mistery.

In previous version of my patch I was not invoking do_CreateInstance("@mozilla.org/image/tools;1"); before doing any of the off main thread decoding, then Andrew suggested me to add that when I hit some assertions related to not being on the MainThread.
I never connected the fact the same issue could cause this one, indeed what happens is that CanHold hits this code path:

SurfaceCache::CanHold(size_t aSize)
{
  StaticMutexAutoLock lock(sInstanceMutex);
  if (!sInstance) {
=>    return false;

Looks like there is no instance because I didn't cause initialization of the whole library, thus CanHold always returns false.
This also explains why my newer patch doesn't hit this, there I added the image/tools instance initialization.
Thank you for all the help figuring this out.

Now the final question, should I backout the patch in this bug since the problem was caused by a mistake in my code?
Flags: needinfo?(mak77) → needinfo?(tnikkel)
Thanks for figuring that out!

I suppose we should change the code to return a failure nsresult if the source buffer is marked complete before the local code calls complete. And maybe add a warning.
Flags: needinfo?(tnikkel)
(Assignee)

Updated

6 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

6 months ago
Created attachment 8840923 [details] [diff] [review]
ImageOps:DecodeToSurface may assert on a complete Sourcebuffer if ImageLib was not initialized

MozReview-Commit-ID: 5GBMnretTbS
(Assignee)

Comment 13

6 months ago
Created attachment 8840924 [details] [diff] [review]
ImageOps:DecodeToSurface may assert on a complete Sourcebuffer if ImageLib was not initialized

tip
(Assignee)

Updated

6 months ago
Attachment #8840923 - Attachment is obsolete: true
(Assignee)

Comment 14

6 months ago
Comment on attachment 8840924 [details] [diff] [review]
ImageOps:DecodeToSurface may assert on a complete Sourcebuffer if ImageLib was not initialized

Something like this?

I can't return an error since this returns an already_AddRefed, I'm just returning nullptr.
Attachment #8840924 - Flags: review?(tnikkel)
Comment on attachment 8840924 [details] [diff] [review]
ImageOps:DecodeToSurface may assert on a complete Sourcebuffer if ImageLib was not initialized

Thanks!
Attachment #8840924 - Flags: review?(tnikkel) → review+

Comment 16

6 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7207e80b80
ImageOps:DecodeToSurface may assert on a complete Sourcebuffer if ImageLib was not initialized. r=tnikkel

Comment 17

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd7207e80b80
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.