1139225 - Add locking for imgRequest member variables that can be accessed from multiple threads

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

As explained in bug 1137002, we currently have racy accesses to imgRequest member variables from multiple threads. These racy accesses all originate from the fact that imgRequest::OnDataAvailable (almost) always runs off-main-thread now, but touches member variables that also get accessed from the main thread.

I've tried to separate out what I could, but the remaining task is nontrivial: we need to ensure that all of those accesses are synchronized, with no significant performance impact, and do so in a way that's maintainable.

Given how ugly the original code is, the maintainability part in particular is a bit tricky. My highest priority is to simplify reasoning about this code in the future from a concurrency standpoint, so an important part of this patch will be to try to isolate the code in OnDataAvailable that needs to deal with synchronization in one place, and disentangle it from the other logic. I've already spent more time than is really justified fussing around with the exact structure of the code, so I don't want to aim for perfection here, but hopefully the final state of the code will be something that is at least easier to reason about when refactoring further.

Comment 1

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

Attachment: (Part 1) - Remove unused imgRequest::mBlockingOnload flag

Part 1 just removes an unused flag so we don't have to worry about it in the
changes that follow. This is so trivial it didn't seem to justify its own bug.

Comment 2

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

Attachment: (Part 2) - Dispatch OnImageAvailable to the main thread manually in imgRequest

In this part we stop automatically forwarding ProgressTracker::OnImageAvailable
to the main thread.

The new code in imgRequest will be immediately replaced in part 3; the
motivation here is to make it possible to merge the
ProgressTracker::OnImageAvailable runnable and the imgRequest::SetProperties
runnable (SetPropertiesEvent) , since in part 3 we need to rework that runnable
anyway and it's more efficient to merge them.

Comment 3

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

Attachment: (Part 3) - Make OnDataAvailable threadsafe

OK, here's part 3. I'd really have liked there to be more parts, but it was
tricky in practice to split a lot of this stuff out since many of the changes
depend on each other.

The most important changes are:

- We add a mutex to each imgRequest. Several of the member variables are now
  protected by this mutex. (The list is documented in imRequest.h.)
 
- Since virtually all of the flags needed to be protected, and things that are
  protected cannot share the same bitfield with things that aren't, I decided to
  just make all flags protected. The only flag for which this required adding
  new, in some sense unnecessary locking was mIsInCache, but this is a
  time/space tradeoff; storing mIsInCache separately to avoid the lock would
  have increased the size of every imgRequest object, and I don't anticipate the
  methods that touch it being called very often at all, or there being any lock
  contention resulting from this change.

- I added a private method, imgRequest::GetImage(), because there were multiple
  places where we only took the lock to grab the current value of mImage.

- As mentioned in comment 0, it was important to me to separate the
  synchronization logic in OnDataAvailable from the other logic, so I took the
  approach of making the code that's directly in OnDataAvailable mostly about
  synchronization. OnDataAvailable now delegates the real work to other code,
  most importantly the new function imgRequest::PrepareForNewPart.

- PrepareForNewPart does all of the work that OnDataAvailable did when
  mNewPartPending was set. Because it's not a method of imgRequest, we can be
  sure that it doesn't engage in any inappropriate mutation of member variables.
  That should make it harder to mess this up again in the future. Instead,
  OnDataAvailable passes in the state that PrepareForNewPart needs, and it
  returns all of the new state that we need to update imgRequest with using the
  new type NewPartResult.

- As I mentioned in the comment for part 2, the runnables used for
  OnImageAvailable and SetProperties are merged into a single runnable,
  FinishPreparingForNewPartRunnable. This runnable takes ownership of the
  NewPartResult that PrepareForNewPart returned, and takes care of any final,
  main-thread-only work. Some other code had to be updated to make this work
  smoothly - in particular, SetProperties needed a different signature.

After this patch, we will be in a position where all the state that
OnDataAvailable touches is synchronized. Indeed, this patch is pretty
conservative about that, because I don't see any reason to behave otherwise
unless there's a performance benefit - we could, for example, rely on the
implicit synchronization happening elsewhere between OnStartRequest and
OnDataAvailable and touch mIsMultiPartChannel outside of the lock, but there
would be no performance benefit, and it would make the code harder to reason
about.

I do feel like things are still uglier than I'd like them to be, but we'll
continue tweaking and refactoring this code over time.

Comment 4

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

One thing I forgot to mention: FinishPreparingForNewPart is also responsible for updating imgRequest::mContentType. That's another reason I wanted to merge all these different runnables; we would've had three total if each task got a separate runnable.

Comment 5

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

Attachment: (Part 1) - Remove unused imgRequest::mBlockingOnload flag

Updated part 1 to also remove method declarations for onload-blocking-related
methods that aren't defined anywhere.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8572313 [details] [diff] [review]
(Part 3) - Make OnDataAvailable threadsafe

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

::: image/src/imgRequest.cpp
@@ +426,5 @@
> +  bool isInCache = false;
> +
> +  {
> +    MutexAutoLock lock(mMutex);
> +    isInCache = mIsInCache;

Is it safe with this small scope of mutex while we call this and imgRequest::SetIsInCache() at the same time?

Comment 7

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

Pushed a try job:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35d58ca2e4e6

Comment 8

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

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #6)
> Is it safe with this small scope of mutex while we call this and
> imgRequest::SetIsInCache() at the same time?

Yes, the small scope is intentional. See the second bullet point in comment 3. mIsInCache is not accessed anywhere but on the main thread. However, it's a member of a bitfield which is guarded by the lock. In order for that to be safe, all bitfield members must only be accessed with the lock held.

Comment 9

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

Attachment: (Part 3) - Make OnDataAvailable threadsafe

This new version of part 3 fixes two issues:

- The switch from StartDecoding to RequestDecode meant that we were calling a
  method that wasn't supposed to be running off-main-thread, and dying with an
  assertion. This was fixed for StartDecoding by dispatching a runnable to the
  main thread if necessary, but RequestDecode doesn't do that. I've addressed
  this by running RequestDecode in FinishPreparingForNewPart, which consolidates
  yet another runnable into the FinishPreparingForNewPartRunnable. (In a
  followup I'm going to remove the code in StartDecoding that dispatches a
  runnable to the main thread if necessary, since I'm pretty sure it only exists
  to support this one call.)

- There was a typo in the code that called RequestDecode in OnDataAvailable; we
  were calling it via |result.mImage| after calling |result.mImage.forget()|,
  when we really should've been calling it via |image|. This got fixed by the
  fix for the other issue. This problem was the thing that was making everything
  red and orange on try.

Comment 10

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

(Bug 1141819 is the followup about removing the implicit runnable dispatch in StartDecoding.)

Comment 11

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

Here's a new try job:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86a7d9015abb

Comment 12

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

The new try job is looking pretty green.

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8575629 [details] [diff] [review]
(Part 3) - Make OnDataAvailable threadsafe

> NS_IMETHODIMP imgRequest::OnStartRequest(nsIRequest *aRequest, nsISupports *ctxt)
>   if (!mRequest) {
>-    NS_ASSERTION(mpchan,
>-                 "We should have an mRequest here unless we're multipart");
>-    nsCOMPtr<nsIChannel> chan;
>-    mpchan->GetBaseChannel(getter_AddRefs(chan));
>-    mRequest = chan;
>+    nsCOMPtr<nsIMultiPartChannel> multiPartChannel = do_QueryInterface(aRequest);
>+    MOZ_ASSERT(multiPartChannel, "Should have mRequest unless we're multipart");
>+    nsCOMPtr<nsIChannel> baseChannel;
>+    multiPartChannel->GetBaseChannel(getter_AddRefs(baseChannel));
>+    mRequest = baseChannel;
>   }

We already have multiPartChannel defined above in the same way in this function. Why can't we use the existing one?

Comment 14

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

Attachment: (Part 3) - Make OnDataAvailable threadsafe

Rebased.

Comment 15

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

Pushed to inbound:

3f5eae4fac0f	Seth Fowler — Bug 1139225 (Part 3) - Make OnDataAvailable threadsafe. r=tn
fcaed7860ea4	Seth Fowler — Bug 1139225 (Part 2) - Dispatch OnImageAvailable to the main thread manually in imgRequest. r=tn
fbd89012eda7	Seth Fowler — Bug 1139225 (Part 1) - Remove unused imgRequest::mBlockingOnload flag. r=tn

Comment 16

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

(In reply to Timothy Nikkel (:tn) from comment #13)
> We already have multiPartChannel defined above in the same way in this
> function. Why can't we use the existing one?

Thanks for the review!

I didn't want to fix this and have to run another huge try job, but I will push a followup that fixes it.

Comment 17

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

Pushed a followup to replace MOZ_OVERRIDE with override and MOZ_FINAL with final:

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbd89012eda7
https://hg.mozilla.org/mozilla-central/rev/fcaed7860ea4
https://hg.mozilla.org/mozilla-central/rev/3f5eae4fac0f
https://hg.mozilla.org/mozilla-central/rev/f687743cc499

Comment 19

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

Attachment: Followup - Remove duplicate multiPartChannel variable

This followup addresses Timothy's review comment above by removing the duplicate
multiPartChannel variable from imgRequest::OnStartRequest.

Comment 20

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

Pushed the followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae8156e0956

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ae8156e0956