Closed Bug 940714 Opened 6 years ago Closed 6 years ago

Add a RAII class to make synchronous decoding safer

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox-esr24 --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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.
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
Attachment #8334918 - Flags: review?(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?
(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.
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 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?
Renamed AutoSyncDecode to AutoSetSyncDecode.
Attachment #8334934 - Flags: review?(tnikkel)
Attachment #8334918 - Attachment is obsolete: true
Attachment #8334918 - Flags: review?(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.
(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.
(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.
(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?
That sounds like a good plan to me.
This patch implements the plan in comment 10.
Attachment #8334995 - Flags: review?(tnikkel)
Attachment #8334934 - Attachment is obsolete: true
Attachment #8334934 - Flags: review?(tnikkel)
Ended up making it private because, on reflect, I don't think Decoder subclasses should mess with it either.
Attachment #8334995 - Flags: review?(tnikkel) → review+
Thanks for the review, Timothy!
https://hg.mozilla.org/mozilla-central/rev/b0433773e27c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
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.
Attachment #8334995 - Flags: approval-mozilla-release?
Attachment #8334995 - Flags: approval-mozilla-esr24?
Attachment #8334995 - Flags: approval-mozilla-beta?
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
Attachment #8334995 - Flags: approval-mozilla-release?
Attachment #8334995 - Flags: approval-mozilla-release+
Attachment #8334995 - Flags: approval-mozilla-esr24?
Attachment #8334995 - Flags: approval-mozilla-esr24+
Attachment #8334995 - Flags: approval-mozilla-beta?
Attachment #8334995 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.