Causes an assertion similar to bug 1238574.

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?

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.

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

(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.

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?

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?

(On a modern laptop I'd expect the size of the surface cache to be around 1 GB, so 2550 bytes is way small.)

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?

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.

MozReview-Commit-ID: 5GBMnretTbS

tip

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.

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

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