Bug 227471

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=227664

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Ryosuke Niwa

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2021-06-28 17:13:16 PDT

<rdar://79694015>

Ryosuke Niwa

Comment 2 2021-06-28 17:19:21 PDT

Created attachment 432445 [details] Patch

Geoffrey Garen

Comment 3 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.

Ryosuke Niwa

Comment 4 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.

Ryosuke Niwa

Comment 5 2021-06-29 03:19:24 PDT

Created attachment 432466 [details] Patch

Geoffrey Garen

Comment 6 2021-06-29 11:11:08 PDT

Comment on attachment 432466 [details] Patch EWS failure looks real.

Ryosuke Niwa

Comment 7 2021-06-29 17:38:37 PDT

Created attachment 432560 [details] Patch

Geoffrey Garen

Comment 8 2021-06-30 16:13:57 PDT

Comment on attachment 432560 [details] Patch r=me