940714 - Add a RAII class to make synchronous decoding safer

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

It was noticed in bug 896268 that if certain errors occur, we may call Decoder::SetSynchronous(true) without a corresponding Decoder::SetSynchronous(false), which can cause us to try to allocate frames off-main-thread despite the danger.

This bug is about adding a RAII class to ensure that SetSynchronous(true) is always paired with SetSynchronous(false) when this is intended.

Comment 1

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

Attachment: Add a RAII class to make synchronous decoding safer.

This patch adds the RAII class and updates callers in RasterImage to use it.

Try job here:
https://tbpl.mozilla.org/?tree=Try&rev=04ff3f00a017

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8334918 [details] [diff] [review]
Add a RAII class to make synchronous decoding safer.

AutoSetSyncDecode sounds like a better name to me.

In SyncDecode would it be better to include FlushInvalidations and FinishedSomeDecoding in the block with the AutoSyncDecode so that the SetSynchronous(false) still happens after them?

Comment 3

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

(In reply to Timothy Nikkel (:tn) from comment #2)
> Comment on attachment 8334918 [details] [diff] [review]
> Add a RAII class to make synchronous decoding safer.
> 
> AutoSetSyncDecode sounds like a better name to me.

Alright, I'll make that change.

> In SyncDecode would it be better to include FlushInvalidations and
> FinishedSomeDecoding in the block with the AutoSyncDecode so that the
> SetSynchronous(false) still happens after them?

You'll notice that the point at which we called SetSynchronous(false) wasn't consistent between the different cases in the original code. I'd like to make things consistent, and the consistent approach I've chosen is to have the scope be as small as possible. If there's a problem with doing things that way, I want to find out.

Comment 4

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

I think it's very, very likely that there is no problem, but recent experience from other bugs shows that if you allow these differences to remain, code can start to depend on them.

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8334918 [details] [diff] [review]
Add a RAII class to make synchronous decoding safer.

>@@ -2395,40 +2390,40 @@ RasterImage::SyncDecode()
>   // If we don't have a decoder, create one
>   if (!mDecoder) {
>     rv = InitDecoder(/* aDoSizeDecode = */ false, /* aIsSynchronous = */ true);
>     CONTAINER_ENSURE_SUCCESS(rv);
>-  } else {
>-    mDecoder->SetSynchronous(true);
>   }
> 
>-  // Write everything we have
>-  rv = DecodeSomeData(mSourceData.Length() - mBytesDecoded);
>-  CONTAINER_ENSURE_SUCCESS(rv);
>+  {
>+    AutoSyncDecode syncDecode(mDecoder);
>+
>+    // Write everything we have
>+    rv = DecodeSomeData(mSourceData.Length() - mBytesDecoded);
>+    CONTAINER_ENSURE_SUCCESS(rv);
>+  }

So if !mDecoder then InitDecoder will call SetSynchronous(true) on the decoder before it returns. When AutoSyncDecode comes into scope it will save the original value, true, and when it goes out of scope it will restore true as for synchronous. Leaving the decoding in sync mode. Or did I miss something?

Comment 6

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

Attachment: Add a RAII class to make synchronous decoding safer.

Renamed AutoSyncDecode to AutoSetSyncDecode.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #3)
> You'll notice that the point at which we called SetSynchronous(false) wasn't
> consistent between the different cases in the original code. I'd like to
> make things consistent, and the consistent approach I've chosen is to have
> the scope be as small as possible. If there's a problem with doing things
> that way, I want to find out.

Okay, that makes sense.

Comment 8

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

(In reply to Timothy Nikkel (:tn) from comment #5)
> So if !mDecoder then InitDecoder will call SetSynchronous(true) on the
> decoder before it returns. When AutoSyncDecode comes into scope it will save
> the original value, true, and when it goes out of scope it will restore true
> as for synchronous. Leaving the decoding in sync mode. Or did I miss
> something?

You didn't miss anything - that's right. There's no problem with a decoder which was created as synchronous remaining synchronous. However, if a decoder was created as non-synchronous, it may have decode jobs in the thread pool. When those jobs actually get run, if the decoder now thinks its a synchronous decoder it'll try to allocate frames off the main thread. This is unsafe, and it's happening in bug 896268.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #8)
> (In reply to Timothy Nikkel (:tn) from comment #5)
> > So if !mDecoder then InitDecoder will call SetSynchronous(true) on the
> > decoder before it returns. When AutoSyncDecode comes into scope it will save
> > the original value, true, and when it goes out of scope it will restore true
> > as for synchronous. Leaving the decoding in sync mode. Or did I miss
> > something?
> 
> You didn't miss anything - that's right. There's no problem with a decoder
> which was created as synchronous remaining synchronous. However, if a
> decoder was created as non-synchronous, it may have decode jobs in the
> thread pool. When those jobs actually get run, if the decoder now thinks its
> a synchronous decoder it'll try to allocate frames off the main thread. This
> is unsafe, and it's happening in bug 896268.

What if we try to do some non-sync decoding after SyncDecode? The decoder will still be in sync mode.

Comment 10

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

(In reply to Timothy Nikkel (:tn) from comment #9)
> What if we try to do some non-sync decoding after SyncDecode? The decoder
> will still be in sync mode.

Hmmm. Initially I thought you were proposing a behavior change, but now I see that the original code did unconditionally call SetSynchronous(false). Furthermore, this is the only place where we create a decoder which is initially synchronous. There is a lot of nastiness going on here which needs to be cleaned up.

I'm going to make the following changes to bring some sanity here:

- AutoSetSyncDecode is the _only_ thing which can call SetSynchronous. SetSynchronous becomes protected and AutoSetSyncDecode becomes a friend class of Decoder.
- The argument to InitDecoder to specify whether the decoder should initially be in the synchronous state is removed. You cannot create a decoder in the synchronous state.

After these changes, it will never be possible for a decoder to be in the synchronous state unless the code in question is guarded by AutoSetSyncDecode.

I initially considered unconditionally calling SetSynchronous(false) in ~AutoSetSyncDecode, but that would introduce bugs if we ever nested two AutoSetSyncDecode instances. Better just to continue restoring the previous state.

Any additional thoughts?

Comment 11

[Timothy Nikkel (:tnikkel)]

That sounds like a good plan to me.

Comment 12

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

Attachment: Add a RAII class to make synchronous decoding safer.

This patch implements the plan in comment 10.

Comment 13

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

Ended up making it private because, on reflect, I don't think Decoder subclasses should mess with it either.

Comment 14

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

Thanks for the review, Timothy!

Comment 15

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

Pushed:

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

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

Comment on attachment 8334995 [details] [diff] [review]
Add a RAII class to make synchronous decoding safer.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's a dependency of the patch in bug 896268.
User impact if declined: Can't uplift bug 896268. See that bug for more info.
Fix Landed on Version: 29
Risk to taking this patch (and alternatives if risky): Very low. On m-c for 3 months with no issues. Affects a section of the code which hasn't churned much since the ESR release, so surprising interactions are unlikely.
String or UUID changes made by this patch: None.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8334995 [details] [diff] [review]
Add a RAII class to make synchronous decoding safer.

approving for uplift so we can get bug 896268 landed

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/fbfb39168bb8
https://hg.mozilla.org/releases/mozilla-esr24/rev/db8014c3d70d