Bug 227471 - RemotePlayback must keep its media element alive when there is a pending activity
Summary: RemotePlayback must keep its media element alive when there is a pending acti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-28 17:10 PDT by Ryosuke Niwa
Modified: 2021-07-03 09:54 PDT (History)
13 users (show)

See Also:


Attachments
Patch (46.62 KB, patch)
2021-06-28 17:19 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (47.04 KB, patch)
2021-06-29 03:19 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (47.11 KB, patch)
2021-06-29 17:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-06-28 17:10:15 PDT
When there is a pending activity in RemotePlayback (e.g. watchAvailability has been called but not yet completed)
then it must keep its media element alive by adding it as an opaque root to GC.
Comment 1 Ryosuke Niwa 2021-06-28 17:13:16 PDT
<rdar://79694015>
Comment 2 Ryosuke Niwa 2021-06-28 17:19:21 PDT
Created attachment 432445 [details]
Patch
Comment 3 Geoffrey Garen 2021-06-28 17:57:07 PDT
Comment on attachment 432445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432445&action=review

> Source/WebCore/Modules/remoteplayback/RemotePlayback.idl:39
> +    GenerateIsReachable=ImplOwnerNodeRoot,
> +    GenerateAddOpaqueRoot=ownerNodeConcurrently,

At first glance, this strikes me as incorrect.

An opaque root should be the *root* of a tree of objects. For example, root(ownerNode()), which is usually the document, is an appropriate opaque root. 

In this case, even though our isReachable() function specifies root(ownerNode()), our addOpaqueRoot() function specifies just plain ownerNode(). This means that, if RemotePlaybackState is the last reference to a disconnected tree of nodes, or to a document, it will not cause the tree of nodes or document to be retained. Perhaps this is an edge case that is not super observable in practice. But I thought I'd point it out.
Comment 4 Ryosuke Niwa 2021-06-29 03:15:34 PDT
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 432445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432445&action=review
> 
> > Source/WebCore/Modules/remoteplayback/RemotePlayback.idl:39
> > +    GenerateIsReachable=ImplOwnerNodeRoot,
> > +    GenerateAddOpaqueRoot=ownerNodeConcurrently,
> 
> At first glance, this strikes me as incorrect.
> 
> An opaque root should be the *root* of a tree of objects. For example,
> root(ownerNode()), which is usually the document, is an appropriate opaque
> root. 
> 
> In this case, even though our isReachable() function specifies
> root(ownerNode()), our addOpaqueRoot() function specifies just plain
> ownerNode(). This means that, if RemotePlaybackState is the last reference
> to a disconnected tree of nodes, or to a document, it will not cause the
> tree of nodes or document to be retained. Perhaps this is an edge case that
> is not super observable in practice. But I thought I'd point it out.

Yikes, you're right. Fixed it and added an extra assertion for this in the test.
Comment 5 Ryosuke Niwa 2021-06-29 03:19:24 PDT
Created attachment 432466 [details]
Patch
Comment 6 Geoffrey Garen 2021-06-29 11:11:08 PDT
Comment on attachment 432466 [details]
Patch

EWS failure looks real.
Comment 7 Ryosuke Niwa 2021-06-29 17:38:37 PDT
Created attachment 432560 [details]
Patch
Comment 8 Geoffrey Garen 2021-06-30 16:13:57 PDT
Comment on attachment 432560 [details]
Patch

r=me
Comment 9 Ryosuke Niwa 2021-06-30 16:53:20 PDT
Comment on attachment 432560 [details]
Patch

Clearing flags on attachment: 432560

Committed r279443 (239299@main): <https://commits.webkit.org/239299@main>
Comment 10 Ryosuke Niwa 2021-06-30 16:53:22 PDT
All reviewed patches have been landed.  Closing bug.