Closed Bug 1372066 Opened 2 years ago Closed 2 years ago

Fix animations leak on abnormal shutdown

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 3 obsolete files)

There is a possibility of animation leak.
Assignee: nobody → sotaro.ikeda.g
Attachment #8876572 - Attachment is obsolete: true
Attachment #8876573 - Flags: review?(nical.bugzilla)
Blocks: 1335335
Comment on attachment 8876573 [details] [diff] [review]
patch - Fix possible Animation leak

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

I am not found of adding a layer of hash maps for every resource only to make sure they are all cleaned up when we tear down the WebRenderBridgeParent. It looks like non-webrender code paths get away with animated values without doing this, but I don't know this code well enough to be sure if/how we could refactor this code so that we don't need this. Moving the review to Kats for this patch and the other one.

::: gfx/layers/wr/WebRenderBridgeParent.h
@@ +245,4 @@
>    std::vector<wr::ImageKey> mKeysToDelete;
>    // XXX How to handle active keys of non-ExternalImages?
>    nsDataHashtable<nsUint64HashKey, wr::ImageKey> mActiveKeys;
> +  nsDataHashtable<nsUint64HashKey, uint64_t> mActiveAnimations;

I wonder if we have a hash set type to use instead of a hash table. the js engine has its own but I don't know if we can/should use it.
Attachment #8876573 - Flags: review?(nical.bugzilla) → review?(bugmail)
Comment on attachment 8876573 [details] [diff] [review]
patch - Fix possible Animation leak

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

Please add a commit message explaining why this is needed and what circumstances trigger the leak. Bug 1357320 added code to make sure all the animations get cleaned up so if that's failing because we missed a scenario it should be documented somewhere what that scenario is.
Attachment #8876573 - Flags: review?(bugmail) → review-
(In reply to Nicolas Silva [:nical] from comment #4)
> Comment on attachment 8876573 [details] [diff] [review]
> patch - Fix possible Animation leak
> 
> Review of attachment 8876573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not found of adding a layer of hash maps for every resource only to
> make sure they are all cleaned up when we tear down the
> WebRenderBridgeParent.

It is also necessary to make "Tab move between different windows(Bug 1335335)".

> It looks like non-webrender code paths get away with
> animated values without doing this, but I don't know this code well enough
> to be sure if/how we could refactor this code so that we don't need this.

In non-webrender case, showed Layer holds related values.
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> (In reply to Nicolas Silva [:nical] from comment #4)
> > Comment on attachment 8876573 [details] [diff] [review]
> > patch - Fix possible Animation leak
> > 
> > Review of attachment 8876573 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I am not found of adding a layer of hash maps for every resource only to
> > make sure they are all cleaned up when we tear down the
> > WebRenderBridgeParent.
> 
> It is also necessary to make "Tab move between different windows(Bug
> 1335335)".
> 

And I do not think it is a good idea not to fix a memory leak.
(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> 
> And I do not think it is a good idea not to fix a memory leak.

If client side is shut down abnormally or killed in the middle of shut down, it causes memory leak.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> Comment on attachment 8876573 [details] [diff] [review]
> patch - Fix possible Animation leak
> 
> Review of attachment 8876573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
Bug 1357320 added code to make sure all the
> animations get cleaned up so if that's failing because we missed a scenario
> it should be documented somewhere what that scenario is.

Bug 1357320 should clean up all animation in normal shut down case. But Bug 1357320 has a problem and causes Bug 1372409.
Bug 1372409 Comment 3 explains the causes of Bug 1372409.

This bug handles abnormal client shut down case and Tab move between different windows(Bug 1335335).

Abnormal client shut down case, WebRenderBridgeParent does not receive IPC messages that are sent during WebRenderLayerManager and WebRenderBridgeChild destruction. In this case, Animations under the WebRenderBridgeChild are not removed from CompositorAnimationStorage. Abnormal shut down could happen when content process was crashed or content process was killed by ContentParent if shut down takes too long time.

The tab move case, CompositorAnimationStorage is changed from one to different one. Then WebRenderBridgeParent needs to remove all active animations from previous CompositorAnimationStorage.
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5)
> > Comment on attachment 8876573 [details] [diff] [review]
> > patch - Fix possible Animation leak
> > 
> > Review of attachment 8876573 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> Bug 1357320 added code to make sure all the
> > animations get cleaned up so if that's failing because we missed a scenario
> > it should be documented somewhere what that scenario is.
> 
> Bug 1357320 should clean up all animation in normal shut down case. But Bug
> 1357320 has a problem and causes Bug 1372409.
> Bug 1372409 Comment 3 explains the causes of Bug 1372409.
> 
> This bug handles abnormal client shut down case and Tab move between
> different windows(Bug 1335335).
> 
> Abnormal client shut down case, WebRenderBridgeParent does not receive IPC
> messages that are sent during WebRenderLayerManager and WebRenderBridgeChild
> destruction. In this case, Animations under the WebRenderBridgeChild are not
> removed from CompositorAnimationStorage. Abnormal shut down could happen
> when content process was crashed or content process was killed by
> ContentParent if shut down takes too long time.
> 
I just verified the abnormal shut down by killing the content process and then I saw compositor was still running the animations from killed process. As Sotaro mentioned, we also don't support tab move between different windows for OMTA.
> The tab move case, CompositorAnimationStorage is changed from one to
> different one. Then WebRenderBridgeParent needs to remove all active
> animations from previous CompositorAnimationStorage.

Sotaro, I think you can append comment 9 in your patch.
(In reply to Peter Chang[:pchang] from comment #10)
> > 
> > Abnormal client shut down case, WebRenderBridgeParent does not receive IPC
> > messages that are sent during WebRenderLayerManager and WebRenderBridgeChild
> > destruction. In this case, Animations under the WebRenderBridgeChild are not
> > removed from CompositorAnimationStorage. Abnormal shut down could happen
> > when content process was crashed or content process was killed by
> > ContentParent if shut down takes too long time.
> > 
> I just verified the abnormal shut down by killing the content process and
> then I saw compositor was still running the animations from killed process.
> As Sotaro mentioned, we also don't support tab move between different
> windows for OMTA.

Thanks for the confirmation!

> > The tab move case, CompositorAnimationStorage is changed from one to
> > different one. Then WebRenderBridgeParent needs to remove all active
> > animations from previous CompositorAnimationStorage.
> 
> Sotaro, I think you can append comment 9 in your patch.

Thanks:) I am going to add comment to a next patch.
Summary: Fix possible Animation leak → Fix animations leak on abnormal shutdown
Attachment #8877497 - Flags: review?(bugmail)
Comment on attachment 8877497 [details] [diff] [review]
patch - Fix animations leak on abnormal shutdown and tab move between different windows

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

::: gfx/layers/wr/WebRenderBridgeParent.h
@@ +240,5 @@
>    RefPtr<widget::CompositorWidget> mWidget;
>    RefPtr<wr::WebRenderAPI> mApi;
>    RefPtr<WebRenderCompositableHolder> mCompositableHolder;
>    RefPtr<CompositorVsyncScheduler> mCompositorScheduler;
> +  RefPtr<CompositorAnimationStorage> mAnimStorage;

What is the reason for WebRenderBridgeParent to hold CompositorAnimationStorage?

Now we have one CompositorAnimationStorage per compositor(CompositorBridgeParent), the mActiveAnimations could clean up the animations which belong to its own tab from that storage.
(In reply to Peter Chang[:pchang] from comment #13)
> Comment on attachment 8877497 [details] [diff] [review]
> patch - Fix animations leak on abnormal shutdown and tab move between
> different windows
> 
> Review of attachment 8877497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderBridgeParent.h
> @@ +240,5 @@
> >    RefPtr<widget::CompositorWidget> mWidget;
> >    RefPtr<wr::WebRenderAPI> mApi;
> >    RefPtr<WebRenderCompositableHolder> mCompositableHolder;
> >    RefPtr<CompositorVsyncScheduler> mCompositorScheduler;
> > +  RefPtr<CompositorAnimationStorage> mAnimStorage;
> 
> What is the reason for WebRenderBridgeParent to hold
> CompositorAnimationStorage?

In CompositorBridgeParent::StopAndClearResources(), WebRenderBridgeParent::Destroy() is called with sIndirectLayerTreesLock held.
And in CrossProcessCompositorBridgeParent::GetAnimationStorage(), CompositorBridgeParent::GetIndirectShadowTree() also holds sIndirectLayerTreesLock.

If WebRenderBridgeParent already holds CompositorAnimationStorage, we could avoid dead lock situation. And we could reduce calling CrossProcessCompositorBridgeParent::GetAnimationStorage().
Comment on attachment 8877497 [details] [diff] [review]
patch - Fix animations leak on abnormal shutdown and tab move between different windows

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

So I see how this would fix the abnormal client shut down case. But with the incomplete transaction case, the animations are going to be leaked for the lifetime of the WebRenderBridgeParent, right? Because they will get sent over with the first "real" transaction following the incomplete transaction, and then they will not get cleared until the WebRenderBridgeParent has ClearResources() called which will probably not be until shutdown. Instead, I think a better fix for that scenario is to clear the mParentCommands in WebRenderBridgeChild in the case of an incomplete transaction. Anyway that seems like a bug that we put stuff in mParentCommands while we are processing one transaction and then don't clear it, and then the next transaction just adds to the list. So I think you should have another patch that fixes that (either on this bug or preferably in bug 1372409).

Finally, just so I understand correctly - in the case of the tab move, the code is not written yet, correct? I imagine that when the tab is moved, the WebRenderBridgeParent will get associated with a different CompositorBridgeParent, and you'll need to clear any animations still pending in mActiveAnimations, and then update mAnimStorage to point to the new Animation Storage for that new CompositorBridgeParent. Is that correct? If so, please update the last sentence in your commit message to this: "In the tab move case, the WebRenderBridgeParent will need to be re-bound to a different CompositorBridgeParent and CompositorAnimationStorage, and so will need to clear all its active animations from the old CompositorAnimationStorage and add animations into the new CompositorAnimationStorage. This will happen in a future patch."

r+ with comments addressed, but if I'm wrong about anything please let's discuss more before landing. Thanks!

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +118,5 @@
>    , mWidget(aWidget)
>    , mApi(aApi)
>    , mCompositableHolder(aHolder)
>    , mCompositorScheduler(aScheduler)
> +  , mAnimStorage(aAnimStorage)

Add a MOZ_ASSERT(mAnimStorage) in the constructor.

@@ +1036,5 @@
>  
> +  // Clear active animations
> +  for (auto iter = mActiveAnimations.Iter(); !iter.Done(); iter.Next()) {
> +    mAnimStorage->ClearById(iter.Data());
> +    iter.Remove();

I think it's faster if you don't do the iter.Remove() here and then just do a .clear() on mActiveAnimations after the loop is complete. I don't know if the nsDataHashtable has a Clear() but std::unordered_set definitely does.

::: gfx/layers/wr/WebRenderBridgeParent.h
@@ +246,5 @@
>    // XXX How to handle active keys of non-ExternalImages?
>    nsDataHashtable<nsUint64HashKey, wr::ImageKey> mActiveKeys;
> +  // mActiveAnimations is used to avoid leaking animations when WebRenderBridgeParent is
> +  // destroyed abnormally and Tab move between different windows.
> +  nsDataHashtable<nsUint64HashKey, uint64_t> mActiveAnimations;

I would change this to use a std::unordered_set<uint64_t>. There's no need for a map since the key and values are always the same. Updating the call sites that use this should be straightforward.
Attachment #8877497 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> So I see how this would fix the abnormal client shut down case. But with the
> incomplete transaction case, the animations are going to be leaked for the
> lifetime of the WebRenderBridgeParent, right? Because they will get sent
> over with the first "real" transaction following the incomplete transaction,
> and then they will not get cleared until the WebRenderBridgeParent has
> ClearResources() called which will probably not be until shutdown. Instead,
> I think a better fix for that scenario is to clear the mParentCommands in
> WebRenderBridgeChild in the case of an incomplete transaction. Anyway that
> seems like a bug that we put stuff in mParentCommands while we are
> processing one transaction and then don't clear it, and then the next
> transaction just adds to the list. So I think you should have another patch
> that fixes that (either on this bug or preferably in bug 1372409).

Oh, my bad. I didn't see bug 1372816. It makes sense now, you can ignore this paragraph.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> Comment on attachment 8877497 [details] [diff] [review]
> patch - Fix animations leak on abnormal shutdown and tab move between
> different windows
> 
> Review of attachment 8877497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Finally, just so I understand correctly - in the case of the tab move, the
> code is not written yet, correct?

Yes, it is not written yet.

> I imagine that when the tab is moved, the
> WebRenderBridgeParent will get associated with a different
> CompositorBridgeParent, and you'll need to clear any animations still
> pending in mActiveAnimations, and then update mAnimStorage to point to the
> new Animation Storage for that new CompositorBridgeParent. Is that correct?

Yes.

> If so, please update the last sentence in your commit message to this: "In
> the tab move case, the WebRenderBridgeParent will need to be re-bound to a
> different CompositorBridgeParent and CompositorAnimationStorage, and so will
> need to clear all its active animations from the old
> CompositorAnimationStorage and add animations into the new
> CompositorAnimationStorage. This will happen in a future patch."

I'll update the commit message in a next patch.

> 
> r+ with comments addressed, but if I'm wrong about anything please let's
> discuss more before landing. Thanks!
> 
> ::: gfx/layers/wr/WebRenderBridgeParent.cpp
> @@ +118,5 @@
> >    , mWidget(aWidget)
> >    , mApi(aApi)
> >    , mCompositableHolder(aHolder)
> >    , mCompositorScheduler(aScheduler)
> > +  , mAnimStorage(aAnimStorage)
> 
> Add a MOZ_ASSERT(mAnimStorage) in the constructor.

I'll update in a next patch.

> 
> @@ +1036,5 @@
> >  
> > +  // Clear active animations
> > +  for (auto iter = mActiveAnimations.Iter(); !iter.Done(); iter.Next()) {
> > +    mAnimStorage->ClearById(iter.Data());
> > +    iter.Remove();
> 
> I think it's faster if you don't do the iter.Remove() here and then just do
> a .clear() on mActiveAnimations after the loop is complete. I don't know if
> the nsDataHashtable has a Clear() but std::unordered_set definitely does.
> 
> ::: gfx/layers/wr/WebRenderBridgeParent.h
> @@ +246,5 @@
> >    // XXX How to handle active keys of non-ExternalImages?
> >    nsDataHashtable<nsUint64HashKey, wr::ImageKey> mActiveKeys;
> > +  // mActiveAnimations is used to avoid leaking animations when WebRenderBridgeParent is
> > +  // destroyed abnormally and Tab move between different windows.
> > +  nsDataHashtable<nsUint64HashKey, uint64_t> mActiveAnimations;
> 
> I would change this to use a std::unordered_set<uint64_t>. There's no need
> for a map since the key and values are always the same. Updating the call
> sites that use this should be straightforward.

Thanks for the advice, I'll update in a next patch.
Applied the comments.
Attachment #8877497 - Attachment is obsolete: true
Attachment #8877882 - Flags: review+
Blocks: 1372512
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17a88314c71
Fix animations leak on abnormal shutdown and tab move between different windows r=kats
https://hg.mozilla.org/mozilla-central/rev/e17a88314c71
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.