Summary: | RemotePlayback must keep its media element alive when there is a pending activity | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | Media | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, jer.noble, joepeck, kondapallykalyan, philipj, sam, sergio | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=227664 | ||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2021-06-28 17:10:15 PDT
Created attachment 432445 [details]
Patch
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. (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. Created attachment 432466 [details]
Patch
Comment on attachment 432466 [details]
Patch
EWS failure looks real.
Created attachment 432560 [details]
Patch
Comment on attachment 432560 [details]
Patch
r=me
Comment on attachment 432560 [details] Patch Clearing flags on attachment: 432560 Committed r279443 (239299@main): <https://commits.webkit.org/239299@main> All reviewed patches have been landed. Closing bug. |