New issue
Advanced search Search tips

Issue 848086 link

Starred by 1 user
Status: Fixed
Owner:
kozy@chromium.org
Closed: Aug 21
Cc:
pfeldman@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Should check whether the script is allowed before invoking the mutation observer

Project Member Reported by jnd@chromium.org, May 30 2018 
For a page which uses mutation observer like the following code

  var observer = new MutationObserver(
     (mutations) => {console.log('Mutation observer ran.');});
  observer.observe(document.body, {childList: true});

If the script is disabled from Dev tools>Settings, the following call should not trigger invoking mutation observer .

  document.body.appendChild(document.createTextNode('ha'));

Comment 1 by pfeldman@chromium.org, May 31 2018

Owner: jnd@chromium.org
When script is disabled, page won't be able to run document.body.appendChild. Let's flush out the details of the actual scenario. Do you mean that somewhere between document.body.appendChild and microtasks JS is getting disabled?

Comment 2 by jnd@chromium.org, May 31 2018 

What I meant is that when the script is disabled via DevTools API, some of existing/installed script objects are not aware it if they doesn't check script setting before invoking.

For MutationObserver, it doesn't check script setting before invoking (See [1]).

In this case, once the script is disabled via Chrome inspector, you still can evaluate the script in the page via inspector console. Then the child list change enqueues micro-task of mutation records (See [2]), that you can the mutation observer handle runs again.

One solution may be to disable enqueuing micro-task if the page's script setting is disabled, or add a check in [1].


[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/mutation_observer.cc?rcl=1685e3cb738fcf188b9f052d8e32ebf9f70e02db&l=287

[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/child_list_mutation_scope.cc?rcl=3dec30816333b9c0f82beb2e27cd81afa7e74803&l=127

Comment 3 by pfeldman@chromium.org, May 31 2018

Status: WontFix (was: Assigned)
I'd say that microtasks that follow evaluation should trigger. Otherwise no promise resolution will be happening. We either disable JS and not allow you to run JS or we keep JS enabled and then run microtasks.

I'll close this as won't fix.

Comment 4 by jnd@chromium.org, Jun 2 2018

Status: Available (was: WontFix)
I believe that we still should check the page's script setting before invoking the handler of the mutation observer because the handler can be called even without explicitly running JavaScript.

Please see the attached test case. In the test case, I use DOM parser's yield algorithm to make parsing be done in multiple tasks which trigger multiple calls of mutation observer (See [1]). So if the page's script setting is disabled due to bad behavior in the first call of the mutation observer handler which is done in first task. The rest of tasks can still trigger the call of the mutation observer handler and make the page irresponsible.

Another data point is that DOMTimer does check script setting before executing its scheduled action. (See [2])


[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/html/parser/background_html_parser.cc?rcl=3c58e9e5869e43b2aa5b6bb4360b4028bb3e0466&l=238

[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/scheduled_action.cc?rcl=329ccf907271b326c04c43df8c1e3af2e682a66e&l=110
test_mutation_observer.html
2.7 KB View Download

Comment 6 by pfeldman@chromium.org, Jun 2 2018

Owner: kozy@chromium.org
[1] Seems to be a valid concern, but the fix should probably be on the microtask level, not DOM level.
[2] Is not a valid reference since DOMTimer operates tasks, not microtasks.

But I guess [1] is real and is good enough of a reason to fix it.

Comment 8 by jnd@chromium.org, Jun 5 2018

Status: Fixed (was: Available)

Comment 9 by jnd@chromium.org, Jun 13 2018

Status: Assigned (was: Fixed)
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 20 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a56b451f20b2ec15b8362b35537813611c80232

commit 9a56b451f20b2ec15b8362b35537813611c80232
Author: Johnny(Jianning) Ding <jnd@chromium.org>
Date: Wed Jun 20 12:38:26 2018

Add script setting check in IsCallbackFunctionRunnable.

The incumbent realm schedules the currently-running callback although
it may not correspond to the currently-running function object.
The goal for this CL to is check whether the script setting is disabled
before invoking JS callback, then checking the context which originally
schedules the current-running callback seems to make more sense.

Please notice that this CL changes the behavior of the method
IsCallbackFunctionRunnable to require the execution context of the
incumbent environment to be not empty, not paused and enabled for
JavaScript.

Bug:  849600 , 849601 , 849730 , 848086 , 849915 , 851357 
Change-Id: Id8ab0f9f67f9602635b295b414d2d0026c87e4ab
Reviewed-on: https://chromium-review.googlesource.com/1087536
Commit-Queue: Johnny Ding <jnd@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568799}
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/WebKit/LayoutTests/inspector-protocol/page/not-invoke-mutation-observer-after-script-disabled-expected.txt
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/WebKit/LayoutTests/inspector-protocol/page/not-invoke-mutation-observer-after-script-disabled.js
[add] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/WebKit/LayoutTests/inspector-protocol/resources/mutation-observer-triggered-by-parser.html
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/core/v8/generated_code_helper.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/core/v8/generated_code_helper.h
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/templates/callback_function.cpp.tmpl
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/templates/callback_interface.cpp.tmpl
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_any_callback_function_optional_any_arg.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_long_callback_function.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_string_sequence_callback_function_long_sequence_arg.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_test_callback_interface.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_test_legacy_callback_interface.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_dictionary_arg.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_enum_arg.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_interface_arg.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_test_interface_sequence_arg.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/core/v8_void_callback_function_typedef.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/bindings/tests/results/modules/v8_void_callback_function_modules.cc
[modify] https://crrev.com/9a56b451f20b2ec15b8362b35537813611c80232/third_party/blink/renderer/core/dom/mutation_observer.cc

Comment 11 by kozy@chromium.org, Aug 21

Status: Fixed (was: Assigned)
I believe that this one is fixed.
►

Sign in to add a comment