Bug 225797

Summary:

Add ScriptDisallowedScope to MediaPlayerPrivateAVFoundation

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Media

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Attachments:

Description	Flags
Adds ScriptDisallowedScope	none
Patch for landing	none

Ryosuke Niwa

Reported 2021-05-13 20:27:46 PDT

As a follow up of https://commits.webkit.org/r277379, add ScriptDisallowedScope to a bunch of member functions of MediaPlayerPrivateAVFoundation.

Attachments
Adds ScriptDisallowedScope (4.78 KB, patch) 2021-05-13 20:29 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (4.85 KB, patch) 2021-05-14 14:44 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2021-05-13 20:29:47 PDT

Created attachment 428594 [details] Adds ScriptDisallowedScope

Eric Carlson

Comment 2 2021-05-13 21:19:10 PDT

Comment on attachment 428594 [details] Adds ScriptDisallowedScope View in context: https://bugs.webkit.org/attachment.cgi?id=428594&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3623 > + ScriptDisallowedScope scriptDisallowedScope; Can't this use the less expensive `ScriptDisallowedScope::InMainThread`? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3722 > + ScriptDisallowedScope scriptDisallowedScope; Ditto. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3755 > + ScriptDisallowedScope scriptDisallowedScope; Ditto > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3827 > + ScriptDisallowedScope scriptDisallowedScope; Ditto

Ryosuke Niwa

Comment 3 2021-05-14 00:40:34 PDT

(In reply to Eric Carlson from comment #2) > Comment on attachment 428594 [details] > Adds ScriptDisallowedScope > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428594&action=review > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3623 > > + ScriptDisallowedScope scriptDisallowedScope; > > Can't this use the less expensive `ScriptDisallowedScope::InMainThread`? Is it guaranteed that all these code will always run in the main thread? > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3722 > > + ScriptDisallowedScope scriptDisallowedScope; > > Ditto. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3755 > > + ScriptDisallowedScope scriptDisallowedScope; > > Ditto > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3827 > > + ScriptDisallowedScope scriptDisallowedScope; > > Ditto

Eric Carlson

Comment 4 2021-05-14 08:23:55 PDT

Comment on attachment 428594 [details] Adds ScriptDisallowedScope View in context: https://bugs.webkit.org/attachment.cgi?id=428594&action=review >>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3623 >>> + ScriptDisallowedScope scriptDisallowedScope; >> >> Can't this use the less expensive `ScriptDisallowedScope::InMainThread`? > > Is it guaranteed that all these code will always run in the main thread? Each of these tasks runs inside of an `ensureOnMainThread` block for the same reason you want to disallow script - because they may call up into HTMLMediaElement.

Ryosuke Niwa

Comment 5 2021-05-14 14:41:39 PDT

(In reply to Eric Carlson from comment #4) > Comment on attachment 428594 [details] > Adds ScriptDisallowedScope > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428594&action=review > > >>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3623 > >>> + ScriptDisallowedScope scriptDisallowedScope; > >> > >> Can't this use the less expensive `ScriptDisallowedScope::InMainThread`? > > > > Is it guaranteed that all these code will always run in the main thread? > > Each of these tasks runs inside of an `ensureOnMainThread` block for the > same reason you want to disallow script - because they may call up into > HTMLMediaElement. Oh, right. Will change to use ScriptDisallowedScope::InMainThread.

Ryosuke Niwa

Comment 6 2021-05-14 14:44:48 PDT

Created attachment 428663 [details] Patch for landing

EWS

Comment 7 2021-05-14 15:27:53 PDT

Committed r277512 (237742@main): <https://commits.webkit.org/237742@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428663 [details].

Radar WebKit Bug Importer

Comment 8 2021-05-14 15:28:17 PDT

<rdar://problem/78035454>

Note You need to log in before you can comment on or make changes to this bug.