[WebDriver BiDi] enable and disable scripting via BiDi

[sadym-chromium]

• Enable scripting disabled emulation via WebDriver BiDi. Required for WebDriver BiDi command emulation.setScriptingEnabled.
• Allow bypassDisabledScripting to force-run scripts. Required for some automation scenarios like WebDriver BiDi command script.evaluate.
• Moved check "if scripting is disabled" from "get the current value of the event handler" to "the event handler processing algorithm", so the "get the current value of the event handler" and "event handler IDL attribute" will always return the actual handler, but they will not be processed.
• Removed the step "Check if we can run script" from "8.1.6.6.2 HostEnqueueFinalizationRegistryCleanupJob(finalizationRegistry)" to prevent memory leak.
• Removed the step "Check if we can run script" from "8.1.6.6.4 HostEnqueuePromiseJob(job, realm)" to allow for Promise processing.

Observable effects:

1. on... IDL properties return functions instead of null even if the scripting is off.
2. FinalizationRegistry is triggered even when the JS is turned off.
3. .then works even if the scripting is off.

Tests for effects: web-platform-tests/wpt#55479

---

• At least two implementers are interested (and none opposed):
  • Chromium
  • Mozilla
• Tests are written and can be reviewed and commented upon at:
  • [wdspec] test emulation.setScriptingEnabled web-platform-tests/wpt#54289
• Implementation bugs are filed:
  • Chromium: feat: implement emulation.setScriptingEnabled command GoogleChromeLabs/chromium-bidi#3566
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1982563
  • WebKit: N/A
  • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): N/A
  • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): N/A
• Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A
• MDN issue is filed: N/A
• The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

/infrastructure.html ( diff )
/webappapis.html ( diff )

[sadym-chromium]

@domenic could you please take a look?

[domenic]

The edit to "scripting is disabled" makes sense, but I suspect the bypass is not thorough enough.

If you look at all the call sites for "scripting is disabled" or "scripting is enabled", the ones that worry me are:

• #check-if-we-can-run-script's check means that the script created in script.evaluate() will not actually run. If we ignore that, then the following are still broken:
• #getting-the-current-value-of-the-event-handler's dependency means that inside any script run by script.evaluate(), element.onxyz === null.
• HostEnqueueFinalizationRegistryCleanupJob's dependency means that weak references created by scripts from script.evaluate() will leak forever.
• HostEnqueuePromiseJob's dependency means that promise handlers / async functions spawned from script.evaluate() will never run.

I'm not sure what a good approach that avoids these problems might be.

Maybe, it'd be best to flip this and try to blocklist all the "bad" sources of script, while leaving scripting enabled???? You could try to censor "create a classic script", "create a JavaScript module script", and "create a WebAssembly module script" by having them use the empty string / empty wasm byte sequence. That might catch them all???

But of course, a blocklist approach is fragile in its own way.

I wonder what implementations do?

[sadym-chromium]

@domenic thanks a lot for the detailed analyses!

If you look at all the call sites for "scripting is disabled" or "scripting is enabled", the ones that worry me are:

• #check-if-we-can-run-script's check means that the script created in script.evaluate() will not actually run.

WebDriver BiDi spec invokes EcmaScript's ScriptEvaluation ( scriptRecord ) to run the script. This means the BiDi's scripts should work.

If we ignore that, then the following are still broken:

• #getting-the-current-value-of-the-event-handler's dependency means that inside any script run by script.evaluate(), element.onxyz === null.

I think this is acceptable.

• HostEnqueueFinalizationRegistryCleanupJob's dependency means that weak references created by scripts from script.evaluate() will leak forever.

It looks like we can safely remove the "check if we can run script" step from HostEnqueueFinalizationRegistryCleanupJob, as there is no reason for not cleaning things up.

• HostEnqueuePromiseJob's dependency means that promise handlers / async functions spawned from script.evaluate() will never run.

I assume the "check if we can run script" HostEnqueuePromiseJob is required to stop processing existing scripts if the script was disabled during the realm's lifetime. In Chromium, we don't perform this step.

I'm not sure what a good approach that avoids these problems might be.

Maybe, it'd be best to flip this and try to blocklist all the "bad" sources of script, while leaving scripting enabled???? You could try to censor "create a classic script", "create a JavaScript module script", and "create a WebAssembly module script" by having them use the empty string / empty wasm byte sequence. That might catch them all???

I don't think the blocklist addresses the issue with the JavaScript disabling during some script execution.

But of course, a blocklist approach is fragile in its own way.

I wonder what implementations do?

From the implementation perspective, in Chromium we don't stop processing microtasks queue when the scripting was disabled.

In order to address the mentioned concerns I would propose to:

1. Remove "check if we can run script" from HostEnqueueFinalizationRegistryCleanupJob.
2. Breaking change Remove "check if we can run script" from HostEnqueuePromiseJob

WDYT?

[domenic]

WebDriver BiDi spec invokes EcmaScript's ScriptEvaluation ( scriptRecord ) to run the script. This means the BiDi's scripts should work.

It is a little bit scary that you are not using the encapsulation HTML provides. For example, it looks like you are not handling parse errors (stored in "error to rethrow"). Do you really need the full power of going directly to the ECMAScript spec layer?

I think this is acceptable.

Really? Is that what implementations do?

It looks like we can safely remove the "check if we can run script" step from HostEnqueueFinalizationRegistryCleanupJob, as there is no reason for not cleaning things up.

The issue is that this might run the web developer-registered handlers for WeakRefs.

I don't know if this is a concrete problem, but it is a conceptual one. For example, let's suppose WebDriver BiDi lets you mutably toggle on and off scripting disabled vs. enabled. (I can't tell quickly whether that is true or not, but maybe it is?)

Then, JS code could do

const someObject = {};

const registry = new FinalizationRegistry(() => {
  console.log("foo");
});

registry.register(someObject);

while scripting is enabled. Then, WebDriver BiDi makes scripting disabled. Then, GC runs and collects someObject. At that point, "foo" will be logged, even though scripting is disabled. (Of course, it could be much worse than logging; it could execute arbitrary script.)

I doubt that is what you want?

I assume the "check if we can run script" HostEnqueuePromiseJob is required to stop processing existing scripts if the script was disabled during the realm's lifetime. In Chromium, we don't perform this step.

Or maybe, that is what you want? If you disable script during a realm's lifetime, you are OK with still running script that was registered in promise handlers and weakref finalizers?

Why? Why make exceptions for those constructs, in particular? What about, e.g., normal event listeners? In particular, it seems bad that web APIs that use promises vs. web APIs that use events have completely different behavior when WebDriver BiDi "disables script".

[annevk]

It's unfortunate that both Domenic and bz left the browser space as they would be the best reviewers for this. On the upside it's not web-exposed I suppose.

[sadym-chromium]

... it's not web-exposed I suppose.

@annevk correct, it is not intended to be web exposed

[sadym-chromium]

This is how currently Chrome behaves when emulating disabled JS (web-platform-tests/wpt#55479):

• #getting-the-current-value-of-the-event-handler's dependency means that inside any script run by script.evaluate(), element.onxyz === null.

element.onxyz is NOT null.

• HostEnqueueFinalizationRegistryCleanupJob's dependency means that weak references created by scripts from script.evaluate() will leak forever.

The FinalizationRegistryCleanup WILL be run, even though the scripting is disabled.

• HostEnqueuePromiseJob's dependency means that promise handlers / async functions spawned from script.evaluate() will never run.

The promise chains (.then) are still executed when the scripting is disabled.

[sadym-chromium]

• #getting-the-current-value-of-the-event-handler's dependency means that inside any script run by script.evaluate(), element.onxyz === null.

element.onxyz is NOT null.

To align spec with this behavior, I can move the check "if scripting is disabled" from "get the current value of the event handler" to "the event handler processing algorithm", so the "get the current value of the event handler" and "event handler IDL attribute" will always return the actual handler, but they will not be processed.

• HostEnqueueFinalizationRegistryCleanupJob's dependency means that weak references created by scripts from script.evaluate() will leak forever.

The FinalizationRegistryCleanup WILL be run, even though the scripting is disabled.

To align with the implementation, I can remove the step "Check if we can run script" from "8.1.6.6.2 HostEnqueueFinalizationRegistryCleanupJob(finalizationRegistry)". I don't think this approach exposes new risks, as in order to create "FinalizationRegistry", the script have to be executed. And the arbitrary script can be injected right there.

• HostEnqueuePromiseJob's dependency means that promise handlers / async functions spawned from script.evaluate() will never run.

The promise chains (.then) are still executed when the scripting is disabled.

To align with the current implementation, the check "check if we can run script" should be removed from "8.1.6.6.4 HostEnqueuePromiseJob(job, realm)". The promise jobs are scheduled immediately one after another, so I don't think there is a way to force-stop JS execution between microtasks.

[foolip]

That sounds sensible to me, but where in the event handler processing algorithm will you check if scripting is disabled? Browsers have internal event handlers for lots of things, so those still need to run, it's just event handlers that would run JS callbacks that we should skip.

[sadym-chromium]

That sounds sensible to me, but where in the event handler processing algorithm will you check if scripting is disabled? Browsers have internal event handlers for lots of things, so those still need to run, it's just event handlers that would run JS callbacks that we should skip.

Current spec:

The event handler processing algorithm for an EventTarget object eventTarget, a string name representing the name of an event handler, and an Event object event is as follows:

1. Let callback be the result of getting the current value of the event handler given eventTarget and name.

If scripting is disabled, getting the current value of the event handler returns null.

2. If callback is null, then return.

Proposed spec:

The event handler processing algorithm for an EventTarget object eventTarget, a string name representing the name of an event handler, and an Event object event is as follows:

1. If scripting is disabled for eventTarget, then return.

2. Let callback be the result of getting the current value of the event handler given eventTarget and name.

3. If callback is null, then return.

[foolip]

Hmm, scripting is disabled for nodes, and not all event targets are nodes. Is there a document you can check on instead?

[sadym-chromium]

Hmm, scripting is disabled for nodes, and not all event targets are nodes. Is there a document you can check on instead?

According to "get the current value of the event handler", the EventTarget is either an element or a Window. I can extend "the event handler processing algorithm" with both cases.

[foolip]

If we get the document from window that should work, yes. (Window isn't a Node)

[sadym-chromium]

@foolip done

[foolip]

I'm not sure that the assert will hold.

While searching for terms in the spec I also found https://html.spec.whatwg.org/#killing-scripts which claims that "If scripting is disabled while a script is executing, the script should be terminated immediately." I'm not sure if browsers really do that, can you test it? If we should maintain that idea in the spec, then after all there might be extra work needed to prevent microtasks from running.

[foolip]

This is similar to https://html.spec.whatwg.org/#getting-the-current-value-of-the-event-handler where it is assumed that eventTarget is an element or a document. I guess that assumption is true but didn't check carefully.

This ("the event handler processing algorithm") code seems to reachable for other types, however, via "activate an event handler" which in turn is referenced by by the https://html.spec.whatwg.org/#event-handler-idl-attributes setter. And there are other kinds of objects than nodes which have event handler IDL attribute, for example OffscreenCanvas and CloseWatcher.

Given that there is already a "scripting is disabled" check inside of https://html.spec.whatwg.org/#getting-the-current-value-of-the-event-handler, should we have checks in both places? What additional cases are covered by checking here?

Sorry, I'm not deeply familiar with this, learning as I go...

[sadym-chromium]

In this PR I moved the "scripting is disabled" check from "get the current value of the event handler" to "the event handler processing algorithm", so that the IDL getters and setters are working even if the scripting is disabled. My assumption is that the all the on... events are invoked only by the "The event handler processing algorithm", while IDL setters and getters are used for assigning the handlers, but not invoking them.

[foolip]

Oh, you removed the "If scripting is disabled for document, then return null" step, I missed that. You're indeed moving the check, not adding one.

You're right that the IDL setter is just for adding a handler, not invoking it, but it does create a callback that "executes the steps of the event handler processing algorithm" and the this object can be an OffscreenCanvas, it seems to me.

I think the reason it previously worked is because of the "If eventHandler's value is an internal raw uncompiled handler" condition, which would only be for Element and Window objects, from on... HTML attributes.

What is the expected behavior for normal event handlers registered via addEventListener()? I don't see it tested in web-platform-tests/wpt#54289 or web-platform-tests/wpt#55479 and I'm not sure what the behavior would be in the spec as it is. If only the "internal raw uncompiled handler" case has a scripting is disabled check, then do other event handlers just fire? This should be possible to test with a click event handler and clicking the element with WebDriver.

[sadym-chromium]

I extended the "Scripting is enabled" / "Scripting is disabled" algorithms to accept platform objects. Does it address your concerns about OffscreenCanvas?

[sadym-chromium]

The "sandboxed scripts browsing context flag" check used to be a part of the "Scripting is enabled". Now as the "Scripting is enabled" is removed, we still don't want to allow for sandboxed scripts to have access to the event handlers, so check "sandboxed scripts browsing context flag" here.

[foolip]

I suspect that the sandbox flag check isn't needed for the same reason it could be removed from FinalizationRegistry and microtasks above. For <iframe sandbox> without the "allow-scripts" flag, no scripts can run in the first place.

It's possible I don't understand sandboxing, but we should probably do the same thing in all three of these cases.

[foolip]

@zcorpan can you take a look at this? I've reviewed it and I think it all makes sense and matches the desired behavior now, but I'm worried there are details here that I don't understand all the ways that scripts can run, sandboxing, etc.

[zcorpan]

Domenic's questions about "why" in #11441 (comment) are not well addressed, as far as I can tell. I've discussed with @juliandescottes , he will file an issue that details the use cases and rationale for the exceptions to disabling script.

[sadym-chromium]

Domenic's questions about "why" in #11441 (comment) are not well addressed, as far as I can tell. I've discussed with @juliandescottes , he will file an issue that details the use cases and rationale for the exceptions to disabling script.

I believe this relates to:

Why? Why make exceptions for those constructs, in particular? What about, e.g., normal event listeners? In particular, it seems bad that web APIs that use promises vs. web APIs that use events have completely different behavior when WebDriver BiDi "disables script".

Which in turn relates to:

Removed the step "Check if we can run script" from "8.1.6.6.4 HostEnqueuePromiseJob(job, realm)" to allow for Promise processing.

Basically the question is what should happen if there is an infinite loop with promises, and the user disables the page. In case of Chromium, we don't stop already running JS, but prevent from running those scheduled by setTimeout or setInterval.

[sadym-chromium]

I created another PR #11884 with a bare minimal hooks required for BiDi emulation of disabled JS. I propose to land it first, and to continue discussion on the controversial nuances here. After we have a consensus, we land this PR as a follow-up.