[WebDriver BiDi] enable and disable scripting via BiDi
- Emulate disabled script via WebDriver BiDi. Required for WebDriver BiDi command
emulation.setScriptingEnabled. - Allow
bypassDisabledScriptingto force-run scripts even if the scripting was disabled. Required for some automation scenarios like WebDriver BiDi commandscript.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.
- Added the step "if scripting is disabled" to timer initialisation steps to prevent from scripts running .
Observable effects:
on...IDL properties return functions instead of null even if the scripting is off.FinalizationRegistryis triggered even when the JS is turned off..thenworks even if the scripting is off.setTimeoutandsetIntervaltasks stop working when the JS is turned off.
Tests for effects: https://github.com/web-platform-tests/wpt/pull/55479
- [x] At least two implementers are interested (and none opposed):
- Chromium
- Mozilla
- [x] Tests are written and can be reviewed and commented upon at:
- https://github.com/web-platform-tests/wpt/pull/54289
- [x] Implementation bugs are filed:
- Chromium: https://github.com/GoogleChromeLabs/chromium-bidi/pull/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
- [x] Corresponding HTML AAM & ARIA in HTML issues & PRs: N/A
- [x] MDN issue is filed: N/A
- [x] 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 )
@domenic could you please take a look?
@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 inscript.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 byscript.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:
- Remove "check if we can run script" from
HostEnqueueFinalizationRegistryCleanupJob. - Breaking change Remove "check if we can run script" from
HostEnqueuePromiseJob
WDYT?
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"
HostEnqueuePromiseJobis 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".
... it's not web-exposed I suppose.
@annevk correct, it is not intended to be web exposed
This is how currently Chrome behaves when emulating disabled JS (https://github.com/web-platform-tests/wpt/pull/55479):
#getting-the-current-value-of-the-event-handler's dependency means that inside any script run byscript.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.
#getting-the-current-value-of-the-event-handler's dependency means that inside any script run byscript.evaluate(),element.onxyz === null.
element.onxyzis 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
FinalizationRegistryCleanupWILL 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.
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.
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:
- 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.
- 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:
If scripting is disabled for
eventTarget, then return.Let callback be the result of getting the current value of the event handler given
eventTargetand name.If callback is null, then return.
Hmm, scripting is disabled for nodes, and not all event targets are nodes. Is there a document you can check on instead?
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.
If we get the document from window that should work, yes. (Window isn't a Node)
@foolip done
Observable effects:
on...IDL properties return functions instead of null even if the scripting is off.FinalizationRegistryis triggered even when the JS is turned off..thenworks even if the scripting is off.
@juliandescottes could you please confirm this aligns with what you expect from disabled JS?
(on PTO, can take a look next week). Looking at how Firefox DevTools script disabling behaves, I confirm we expect 1 (el.onsomething returns a function) and 3 (thenables from script.evaluate/callFunction are still called). 2 I assume we want as well as it avoids problematic side effects. In theory, even though we disable scripts initiated by the content page, the ones coming from script.evaluate/callFunction should not be affected.
Maybe @jgraham @lutien @whimboo can also take a look in the meantime.
@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.
Domenic's questions about "why" in https://github.com/whatwg/html/pull/11441#issuecomment-3213299602 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.
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.
I created another PR https://github.com/whatwg/html/pull/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.
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.
https://github.com/whatwg/html/issues/11874
@zcorpan sorry, but I'm a bit confused. Could you please clarify what is missing in this PR which blocks it from being merged?
@sadym-chromium https://github.com/whatwg/html/pull/11441#discussion_r2447986316 is not resolved. Is it intentional that event handler IDL attributes change from null to a function when scripting is disabled because a Document doesn't have a browsing context?
@sadym-chromium #11441 (comment) is not resolved. Is it intentional that event handler IDL attributes change from
nullto a function when scripting is disabled because aDocumentdoesn't have a browsing context?
Extended the answer: https://github.com/whatwg/html/pull/11441/files#r2611021631
Is it intentional that event handler IDL attributes change from
nullto a function when scripting is disabled because aDocumentdoesn't have a browsing context?
I was mistaken, it does still return null. https://github.com/whatwg/html/pull/11441#discussion_r2613919064
I think we should make sure there are tests for this, though. I'll check if there are any, or write new tests.
Tests here https://github.com/web-platform-tests/wpt/pull/56715
Interestingly, browsers don't match the spec for event handler IDL attribute setters. element.onclick = () => ran = true; creates an event handler even though the https://html.spec.whatwg.org/#determining-the-target-of-an-event-handler returns null per spec.
However, that is an issue that already exists in the spec and isn't introduced by this PR, so we can deal with it in a follow-up. I'll file a new issue.