html icon indicating copy to clipboard operation
html copied to clipboard

[WebDriver BiDi] enable and disable scripting via BiDi

Open sadym-chromium opened this issue 5 months ago • 22 comments

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.
  4. setTimeout and setInterval tasks 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 )

sadym-chromium avatar Jul 08 '25 06:07 sadym-chromium

@domenic could you please take a look?

sadym-chromium avatar Aug 13 '25 14:08 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?

sadym-chromium avatar Aug 21 '25 16:08 sadym-chromium

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

domenic avatar Aug 22 '25 07:08 domenic

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

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

sadym-chromium avatar Oct 15 '25 13:10 sadym-chromium

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 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 avatar Oct 16 '25 15:10 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.

sadym-chromium avatar Oct 17 '25 11:10 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.

foolip avatar Oct 17 '25 11:10 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.

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.

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

sadym-chromium avatar Oct 17 '25 11:10 sadym-chromium

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

foolip avatar Oct 17 '25 11:10 foolip

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.

sadym-chromium avatar Oct 17 '25 12:10 sadym-chromium

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

foolip avatar Oct 17 '25 13:10 foolip

@foolip done

sadym-chromium avatar Oct 17 '25 14:10 sadym-chromium

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.

@juliandescottes could you please confirm this aligns with what you expect from disabled JS?

sadym-chromium avatar Oct 20 '25 09:10 sadym-chromium

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

juliandescottes avatar Oct 21 '25 09:10 juliandescottes

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

foolip avatar Oct 21 '25 12:10 foolip

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.

zcorpan avatar Nov 03 '25 13:11 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.

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 avatar Nov 03 '25 16:11 sadym-chromium

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.

sadym-chromium avatar Nov 04 '25 12:11 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.

https://github.com/whatwg/html/issues/11874

zcorpan avatar Nov 19 '25 09:11 zcorpan

@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 avatar Dec 05 '25 09:12 sadym-chromium

@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?

zcorpan avatar Dec 11 '25 14:12 zcorpan

@sadym-chromium #11441 (comment) 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?

Extended the answer: https://github.com/whatwg/html/pull/11441/files#r2611021631

sadym-chromium avatar Dec 11 '25 15:12 sadym-chromium

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?

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.

zcorpan avatar Dec 12 '25 11:12 zcorpan

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.

zcorpan avatar Dec 12 '25 14:12 zcorpan