The DecodeJob objects that RasterImage uses for off-main thread decoding need to be able to schedule new decode jobs if they aren't able to consume all their input data - for example, because decoding crosses a frame boundary. The existing code does this, but asserts or crashes can result if it actually happens. In this bug we fix this by: 1. Removing the assert. 2. Ensuring that we do not tell DecodePool's thread pool to shut down from the main thread while simultaneously sending it new work from a decoder thread, which can cause an assert or a crash.

Proposed patch. This supercedes (and includes) the patch from bug 854835.

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

Try looks fine.

Comment on attachment 732588 [details] [diff] [review] Make it safe to call RasterImage::DecodePool::RequestDecode off the main thread. This seems reasonable, I have one question though. >+ MutexAutoLock threadPoolLock(mThreadPoolMutex); >+ if (mShuttingDown) { >+ // Just drop the job on the floor; we won't need it. >+ } else if (!gMultithreadedDecoding || !mThreadPool) { > NS_DispatchToMainThread(job); > } else { > mThreadPool->Dispatch(job, nsIEventTarget::DISPATCH_NORMAL); > } > } > } Why can't we just use a null mThreadPool instead of the mShuttingDown variable?

(In reply to Timothy Nikkel (:tn) from comment #4) > Why can't we just use a null mThreadPool instead of the mShuttingDown > variable? That's what I wanted to do too, but as you can see in the lines you quoted !mThreadPool is already a possible state that means something else. I don't believe it's hooked up correctly yet but basically in the future that could happen if gMultithreadedDecoding is set to true but we haven't created the new thread pool yet. In that case we have to just send the job to the main thread, since somebody actually needs its results.

(In reply to Seth Fowler [:seth] from comment #5) > (In reply to Timothy Nikkel (:tn) from comment #4) > > Why can't we just use a null mThreadPool instead of the mShuttingDown > > variable? > > That's what I wanted to do too, but as you can see in the lines you quoted > !mThreadPool is already a possible state that means something else. I don't > believe it's hooked up correctly yet but basically in the future that could > happen if gMultithreadedDecoding is set to true but we haven't created the > new thread pool yet. In that case we have to just send the job to the main > thread, since somebody actually needs its results. What it be bad if we just dispatched the job to the main thread in this case too?

(In reply to Timothy Nikkel (:tn) from comment #6) > What it be bad if we just dispatched the job to the main thread in this case > too? I'd prefer to just drop the task, because letting the DecodeJob run will delay shutdown. (It can even potentially submit more DecodeJobs!)

Thanks for the review! Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/503dea706f82

22 change causing both intermittent test failures and web regressions. Tracking. Please nominate a patch for uplift as soon as possible.

Comment on attachment 732588 [details] [diff] [review] Make it safe to call RasterImage::DecodePool::RequestDecode off the main thread. [Approval Request Comment] Bug caused by (feature/regressing bug #): 716140 User impact if declined: Crashes because RequestDecode will attempt to submit jobs even if the decoding thread pool has been shutdown. Testing completed (on m-c, etc.): On m-c since 2013-04-05 with no problems. Risk to taking this patch (and alternatives if risky): I don't feel that I can in good conscience say that any patch involving synchronization between threads is totally without risk, but this patch is small, simple, has been examined thoroughly by multiple people, and has been working well in testing. I strongly doubt that any issue introduced by this patch would be worse than the crash bug it fixes.. String or IDL/UUID changes made by this patch: None.

Once again, thanks Lukas! https://hg.mozilla.org/releases/mozilla-aurora/rev/f15def814184 https://hg.mozilla.org/releases/mozilla-aurora/rev/14d9c6c56dc6

(In reply to Seth Fowler [:seth] from comment #5) > (In reply to Timothy Nikkel (:tn) from comment #4) > > Why can't we just use a null mThreadPool instead of the mShuttingDown > > variable? > > That's what I wanted to do too, but as you can see in the lines you quoted > !mThreadPool is already a possible state that means something else. I don't > believe it's hooked up correctly yet but basically in the future that could > happen if gMultithreadedDecoding is set to true but we haven't created the > new thread pool yet. Crap, wish I had been around to comment on this. This will never ever be the case, so we can go ahead and change this to !mThreadPool in m-c.

This does, unfortunately, just submit the decode job to the main thread, but it also cleans up the code; I'm not sure what's worse.

(In reply to Joe Drew (:JOEDREW! \o/) from comment #14) > Crap, wish I had been around to comment on this. This will never ever be the > case, so we can go ahead and change this to !mThreadPool in m-c. I guess the pref system works differently than how I thought, then. Are you able to install a handler for preference changes? I assume you must be able to, otherwise you wouldn't be able to make that assertion. I wasn't aware of that.

Comment on attachment 738537 [details] [diff] [review] remove mShuttingDown Review of attachment 738537 [details] [diff] [review]: ----------------------------------------------------------------- r+ only if it's possible to install a handle for pref changes that can take the thread pool mutex when changing the state of gMultithreadedDecoding. If that's not possible, I don't see how this is safe, but it sounds from what you're saying like it is.

Actually I take it back. I don't see why this wouldn't be safe since we aren't dropping the job on the floor. Whether it is optimal is a different question (we are potentially delaying shutdown and shifting parallel work to the main thread), but given the granularity of these jobs it's unlikely to matter much.

We *can* add a pref observer regarding gMultithreadedDecoding, but it's significantly more code than just AddBoolVarCache, and as you say, we're not dropping the job on the floor. What I meant by "This will never ever be the case" is that I never ever want us to support changing multithreaded decoding at runtime. We do incidentally support turning it *off* at runtime, because of the use of AddBoolVarCache, but we don't shut down the thread pool when it turns off, and we shouldn't.

Sounds good Joe. With that restriction, it's more clear what's going on.