1291033 - Ensure atomicity of ISurfaceProvider availability and locking status changes

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

SurfaceCache code is not designed to deal with an ISurfaceProvider asynchronously changing from being a placeholder to having an available surface. (The term I'm introducing in the code for this in this bug is "availability" - you're either a placeholder or you're available - so I'll use that here.) Availability changes thus need to be synchronized with the surface cache - in other words, you need to hold the surface cache lock when you're making them.

Similarly, an ISurfaceProvider's locking status (controlled via ISurfaceProvider::SetLocked()) should not change asynchronously. This is currently implicitly the case since nothing except the surface cache code every touches those methods, but we should enforce that.

Comment 1

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

Attachment: (Part 1) - Ensure atomicity of ISurfaceProvider availability changes.

This is the bigger of the two changes. We discussed this on IRC; the idea is to
forbid anything but SurfaceCache code to make changes to the availability of an
ISurfaceProvider, to ensure that we have the strongest possible protection that
it won't happen asynchronously. To that end, we define a new AvailabilityState
type which manages this state and can only be altered by SurfaceCache code.
ISurfaceProviders can start in either start, but can't change states without
calling SurfaceCache::SurfaceAvailable(), which handles the change for them in a
synchronized fashion.

As you can see, this change leaves us in a situation where we have both an old
API for placeholders (SurfaceCache::InsertPlaceholder()) and a new API (usage of
AvailabilityState and SurfaceCache::SurfaceAvailable()). We'll be able to clean
things up and remove code related to the old approach once everything is
converted to the new API, which won't be long. The complete changeover will have
a number of other advantages as well: we'll be able to ensure that all
ISurfaceProvider pointers inside SurfaceCache are NotNull, and we can let
ISurfaceProviders know their own ImageKey and SurfaceKey, which will further
simplify the API.

Note also that SurfaceAvailable() can in theory be implemented more efficiently,
but in the midst of all the other refactorings that are going on, I don't think
it's a good idea to try to be clever. Let's save those optimizations for later.
The current implementation is exactly the same as the old
InsertPlaceholder()-then-later-call-Insert() pattern in terms of efficiency.
(Pretty obviously, since internally it's implemented in almost exactly the same
way.)

Comment 2

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

Attachment: (Part 2) - Ensure atomicity of ISurfaceProvider locking changes.

This is a much simpler change but I think it's wise to remove as many footguns as possible. Now that ISurfaceProviders are exposed outside of the SurfaceCache, we need to forbid non-SurfaceCache code from calling the methods that manipulate locking state. Unfortunately it's harder to wrap this stuff in a special type like we could do for part 1, since locking state may mean something much different to different ISurfaceProvider implementations. For now, at least, I think it's best to continue to implement this via virtual methods, and just make those virtual methods unavailable except to SurfaceCache code.

Comment 3

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

Attachment: (Part 1) - Ensure atomicity of ISurfaceProvider availability changes.

Whoops, reuploading because I noticed that a little of part 2 leaked into part 1.

Comment 4

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

Attachment: (Part 2) - Ensure atomicity of ISurfaceProvider locking changes.

As above.

Comment 5

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

Try job here:

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

Comment 6

[Daniel Holbert [:dholbert]]

(I'll deprioritize this review for the time being, since it sounds like some fixup is coming here, per bug 1291045 comment 10.)

Comment 7

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

Attachment: (Part 1) - Ensure atomicity of ISurfaceProvider availability changes.

I'm not surprised this caused a failure since I accidentally deleted one of the
lines where we take the lock. =) Should be fixed now.

Comment 8

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

Here's a new try job to verify the fix:

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

Comment 9

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

Yep, that was it. The new try job looks good.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8777169 [details] [diff] [review]
(Part 1) - Ensure atomicity of ISurfaceProvider availability changes.

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

r=me with nits addressed:

::: image/SurfaceCache.h
@@ +126,5 @@
> + *
> + * To ensure that availability changes are atomic (and especially that internal
> + * SurfaceCache code doesn't have to deal with asynchronous availability
> + * changes), an ISurfaceProvider which starts as a placeholder can only make the
> + * fact that it now has a surface visible via a call to

Ambiguous-wording nit: Please replace...
 can only make the fact that it now has a surface visible via
...with something like this:
 can only reveal that it now has a surface via

(With the patch's current text, I instinctively want to parse the sentence as "can only make the [...] surface visible", which is not your intended parsing.)

@@ +145,5 @@
> +  explicit AvailabilityState(bool aAvailable) : mAvailable(aAvailable) { }
> +
> +  void SetAvailable() { mAvailable = true; }
> +
> +  bool mAvailable;

Nit: maybe name this mIsAvailable? (adding "Is")

That has a more obvious-boolean flavor.  The current name, "mAvailable", could (for example) be the name of an AvailabilityState object, or a count of available things -- it's not obviously a bool.

@@ +320,5 @@
> +   * becomes locked if the associated image is locked; otherwise, it starts in
> +   * the unlocked state.
> +   *
> +   * If the cache entry containing @aProvider has already been evicted from the
> +   * surface cache, there is no effect.

s/there is/this function has/, perhaps

@@ +323,5 @@
> +   * If the cache entry containing @aProvider has already been evicted from the
> +   * surface cache, there is no effect.
> +   *
> +   * It's illegal to call this function if @aProvider is not a placeholder; by
> +   * definition, it should have a surface available already.

This sentence tripped me up. The second half sounds like it's describing the expected state of |aProvider| (and it kind of is, since aProvider is expected to freshly have a surface available).  But really I think you're trying to elaborate about providers which *shouldn't* be passed in here.

To clarify this text, please replace:
 "it should have"
...with something like:
  "non-placeholder ISurfaceProviders should have"

That makes it clearer what you're explaining, I think.

@@ +331,5 @@
> +   * inserted via InsertPlaceholder(), which then gets replaced by inserting a
> +   * real cache entry with the same keys via Insert()) and the new one (where
> +   * the same cache entry, inserted via Insert(), starts in a placeholder state
> +   * and then transitions to being a normal cache entry via this function). The
> +   * old mechanism will be removed very soon.

(optional): If it's possible to file this "very soon" removal-bug (or a placeholder[1] bug for it) up-front, that would be great, so that we can mention its bug number here in this comment.  (Sometimes it feels like the codebase is littered with vague references to things that will happen "soon", and it's nice to anchor them to bugs where that work is planned, when possible. :))

[1] ha! a placeholder about placeholders.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8776763 [details] [diff] [review]
(Part 2) - Ensure atomicity of ISurfaceProvider locking changes.

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

r=me on part 2

Comment 12

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

Thanks for the reviews! I'll make those changes.

(In reply to Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) from comment #10)
> (optional): If it's possible to file this "very soon" removal-bug (or a
> placeholder[1] bug for it) up-front, that would be great, so that we can
> mention its bug number here in this comment.

Not all of the prerequisite bugs have been filed yet, but I went ahead and filed bug 1292392.

> [1] ha! a placeholder about placeholders.

=)

Comment 13

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

Attachment: (Part 1) - Ensure atomicity of ISurfaceProvider availability changes.

Addressed review comments.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2a5e36e0d3
(Part 1) - Ensure atomicity of ISurfaceProvider availability changes. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0725e82f286
(Part 2) - Ensure atomicity of ISurfaceProvider locking changes. r=dholbert

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a2a5e36e0d3
https://hg.mozilla.org/mozilla-central/rev/f0725e82f286