898569 - Always have the decoder lock when using mInDecoder

Status: RESOLVED INVALID

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

It's not safe to use mInDecoder without holding the lock. While I'm pretty sure it's obsolete (bug 854506), for now it still exists so it should at least be safe.

Comment 1

[Joe Drew (not getting mail)]

Attachment: always lock when you touch mInDecoder

This adds locks everywhere we didn't have them before, and/or moves things.

(I accidentally committed the header file comment that notes mInDecoder is locked in bug 887466.)

 https://tbpl.mozilla.org/?tree=Try&rev=cc32840d707e

Comment 2

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

Comment on attachment 781861 [details] [diff] [review]
always lock when you touch mInDecoder

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

::: image/src/RasterImage.cpp
@@ +817,5 @@
> +  {
> +    MutexAutoLock lock(mDecodingMutex);
> +    if (mInDecoder && (aFlags & imgIContainer::FLAG_SYNC_DECODE))
> +      return NS_ERROR_FAILURE;
> +  }

If you release the lock after reading mInDecoder, what's to stop another thread from immediately setting mInDecoder to true and violating the preconditions of the rest of this method? Everything that requires mInDecoder to remain false to behave correctly needs to be inside the locked region, which may require some code motion.

@@ +882,5 @@
> +  {
> +    MutexAutoLock lock(mDecodingMutex);
> +    if (mInDecoder && (aFlags & imgIContainer::FLAG_SYNC_DECODE))
> +      return NS_ERROR_FAILURE;
> +  }

Looks like the same issue here.

@@ +2597,5 @@
> +  {
> +    MutexAutoLock lock(mDecodingMutex);
> +    if (mInDecoder && (aFlags & imgIContainer::FLAG_SYNC_DECODE))
> +      return NS_ERROR_FAILURE;
> +  }

Same issue here too.

Comment 3

[Joe Drew (not getting mail)]

It's a good point, but I'm not sure whether we actually have anything that requires it; this might actually be a "you're not allowed to do it" error rather than a "we will break if you don't do this" error.

I'll think more about this, though.

Comment 4

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

If it really won't break anything, maybe it should just be an assertion?

Comment 5

[Robert Lickenbrock [:rclick]]

Taking.

Comment 6

[Robert Lickenbrock [:rclick]]

mInDecoder was removed entirely by Bug 1089046, so this is no longer necessary.