HTML5 Media Element default controls do not work if JavaScript is disabled

[dcrousso]

b19e822

HTML5 Media Element default controls do not work if JavaScript is disabled
https://bugs.webkit.org/show_bug.cgi?id=178040
<rdar://problem/32035810>

Reviewed by NOBODY (OOPS!).

* Source/WebCore/bindings/js/ScriptController.h:
* Source/WebCore/bindings/js/ScriptController.cpp:
(WebCore::ScriptController::cacheableBindingRootObject):
(WebCore::ScriptController::bindingRootObject):
(WebCore::ScriptController::jsObjectForPluginElement):
(WebCore::ScriptController::executeScriptInWorld):
(WebCore::ScriptController::canExecuteScripts):
Always execute scripts inside `WebCore::DOMWrapperWorld::Type::Internal` worlds, as they are (often)
necessary for basic builtin functionality (e.g. media controls).

* Source/WebCore/bindings/js/JSEventListener.cpp:
(WebCore::JSEventListener::handleEvent):
* Source/WebCore/bindings/js/JSLazyEventListener.cpp:
(WebCore::JSLazyEventListener::initializeJSFunction const):
(WebCore::JSLazyEventListener::create):
* Source/WebCore/bindings/js/ScheduledAction.cpp:
(WebCore::ScheduledAction::execute):
* Source/WebCore/bindings/js/ScriptControllerMac.mm:
(WebCore::ScriptController::windowScriptObject):
(WebCore::ScriptController::javaScriptContext):
* Source/WebCore/dom/ScriptElement.cpp:
(WebCore::ScriptElement::prepareScript):
* Source/WebCore/html/HTMLCanvasElement.cpp:
(WebCore::HTMLCanvasElement::createElementRenderer):
* Source/WebCore/html/HTMLElement.cpp:
(WebCore::HTMLElement::rendererIsEverNeeded):
* Source/WebCore/html/HTMLIFrameElement.cpp:
(WebCore::isFrameLazyLoadable):
* Source/WebCore/html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::isLazyLoadable const):
* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::controls const):
* Source/WebCore/html/parser/HTMLParserOptions.cpp:
(WebCore::HTMLParserOptions::HTMLParserOptions):
* Source/WebCore/html/parser/HTMLParserScheduler.cpp:
(WebCore::HTMLParserScheduler::shouldYieldBeforeExecutingScript):
* Source/WebCore/inspector/PageDebugger.cpp:
(WebCore::PageDebugger::setJavaScriptPaused):
* Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::buildObjectForEventListener):
* Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:
(WebCore::PageRuntimeAgent::reportExecutionContextCreation):
* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::FrameLoader::willRestoreFromCachedPage):
(WebCore::FrameLoader::dispatchDidClearWindowObjectsInAllWorlds):
(WebCore::FrameLoader::dispatchDidClearWindowObjectInWorld):
* Source/WebKitLegacy/win/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebFrameLoaderClient::dispatchDidClearWindowObjectInWorld):
Pass along the `WebCore::DOMWrapperWorld` when checking whether script execution is possible. If it's
not possible to get a relevant `WebCore::DOMWrapperWorld`, assume the main world (e.g. for DOM stuff).

[geoffreygaren]

I’m not sure this is the right fix. It changes the meaning of the Safari feature that disables JavaScript, arguably introducing a security hole. If Safari wants to disable website JavaScript only, they can do that by setting the scriptMarkupEnabled flag instead.

[dcrousso]

I’m not sure this is the right fix. It changes the meaning of the Safari feature that disables JavaScript, arguably introducing a security hole. If Safari wants to disable website JavaScript only, they can do that by setting the scriptMarkupEnabled flag instead.

but without this then <video controls> is broken/unusable when JS is disabled. how do you suggest that users interact with that if they decide to disable JS?

[whsieh]

I’m not sure this is the right fix. It changes the meaning of the Safari feature that disables JavaScript, arguably introducing a security hole. If Safari wants to disable website JavaScript only, they can do that by setting the scriptMarkupEnabled flag instead.

but without this then <video controls> is broken/unusable when JS is disabled. how do you suggest that users interact with that if they decide to disable JS?

(I suppose the alternative is for Safari to use -[WKWebpagePreferences allowsContentJavaScript] instead? I think this one will still allow UA script execution...)

[dcrousso]

I’m not sure this is the right fix. It changes the meaning of the Safari feature that disables JavaScript, arguably introducing a security hole. If Safari wants to disable website JavaScript only, they can do that by setting the scriptMarkupEnabled flag instead.

but without this then <video controls> is broken/unusable when JS is disabled. how do you suggest that users interact with that if they decide to disable JS?

(I suppose the alternative is for Safari to use -[WKWebpagePreferences allowsContentJavaScript] instead? I think this one will still allow UA script execution...)

Does it also block JS from other non-markup sources (e.g. web extensions)?

IMO the user shouldn't have to suffer because we decided to implement media controls in JS. Yes, I think it's (sadly) expected nowadays that disabling JS will lead to some broken things, but I don't think users expect that to include builtin things too.

Oh BTW it's already the case that some JS is executed even when it's disabled, as the initial DOM structure of the UA shadow inside <video controls> is created. We just don't get anything async after that (e.g. event listeners, timeouts, etc.).

We need at least some basic way to interact with media when JS is disabled. I'm all ears to alternatives if y'all have suggestions :)

[mcatanzaro]

We have other similar bugs that should be considered too:

• User scripts are run even when JavaScript is disabled in settings, but WebPageProxy::runJavaScript is blocked
• Important: Sandbox CSP directives allows websites to block execution of browser features implemented in JavaScript

I believe we could use Devin's work to fix both of these issues as well.

[mcatanzaro]

Regarding the preferences, I'm pretty sure that 98% of apps that want to disable JavaScript really want to:

• Block web content from executing its own JavaScript
• Allow WebKit features that are internally implemented using JavaScript, such as media controls
• Allow WebKit APIs like webkit_web_view_run_javascript/WebPageProxy::runJavaScript. (Surely no preference should ever block this. If an app wanted to not run its own JS, it would simply not call this function.)

The desired behavior pretty closely corresponds to JavaScriptMarkupEnabled preference, not the JavaScriptEnabled preference, which seems not very useful. It's hard to imagine that any app would want to use a setting that blocks all of the above. Apps probably never want to disable their own JavaScript, right? Certainly Safari or any other general-purpose browser should not allow users to mess with JavaScriptEnabled.

P.S. I see there is a warning above the JavaScriptMarkupEnabled preference in WebPreferences.yaml:

# NOTE: Clients should use per-navigation "allowsContentJavaScript" policies instead

but this preference is public API for WPE/GTK, and it is not deprecated, and the per-navigation allowsContentJavaScript stuff is internal-only and not exposed to applications. So using this preference seems practical.

[mcatanzaro]

the per-navigation allowsContentJavaScript stuff is internal-only and not exposed to applications

Actually it is exposed in the Cocoa API, but not for WPE/GTK.

[geoffreygaren]

(I suppose the alternative is for Safari to use -[WKWebpagePreferences allowsContentJavaScript] instead? I think this one will still allow UA script execution...)

Yes.

Does it also block JS from other non-markup sources (e.g. web extensions)?

Depends on how the API client attempts to execute those sources.

IMO the user shouldn't have to suffer because we decided to implement media controls in JS. Yes, I think it's (sadly) expected nowadays that disabling JS will lead to some broken things, but I don't think users expect that to include builtin things too.

I tend to agree that the user experience is not good when you disable JavaScript. I also agree that it's wonky to have an API that says "disable all JavaScript" given that a modern browser engine, or Mail app, or whatever, will tend to implement many of its own features in JavaScript. We might as well have a feature that says "disable all C++!" The problem is, we do have an API that says "disable all JavaScript". Silently changing the meaning of that existing API to enable some kinds of JavaScript even when an API clients asks us not to would be a serious security regression.

That is the conundrum that -[WKWebpagePreferences allowsContentJavaScript] attempts to solve. It specifically distinguishes website JavaScript from app and engine JavaScript, and only disables website JavaScript. (It also has the nifty side benefit that it continues to protect app and engine JavaScript from many kinds of script injection attacks, since it prohibits dynamically generated app and engine markup from being accidentally vivified into a script.)

If Safari wants "Disable JavaScript" to mean "Disable website JavaScript, but keep app and engine JavaScript running", probably the best way to express that is for Safari to adopt -[WKWebpagePreferences allowsContentJavaScript]. But that may imply a number of changes in behavior, not just about media controls, so it's best to get the Safari team's take on whether those changes are what they want. (Another benefit of making a change to Safari instead of WebKit -- the patch review process will find out if it's a change Safari actually wants.)

Oh BTW it's already the case that some JS is executed even when it's disabled, as the initial DOM structure of the UA shadow inside <video controls> is created. We just don't get anything async after that (e.g. event listeners, timeouts, etc.).

Seems like a bug.

[geoffreygaren]

Another option is to offer a third API to control JavaScript execution that explicitly applies controls per isolated world, including the ability to identify any isolated worlds used by WebKit internally. That would also resolve the concern about changing the meaning of a security-sensitive API, and would more closely match the behavior implemented in this patch.

I don't prefer that option because three APIs is worse than two, and I'm not aware of an API client that wants that level of control + fiddliness.