1980766 - Cannot make DevTools stop changing the DOM while it stops JS?

Status: NEW

(DevTools :: Inspector, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

STR

1. Load data:text/html,<div contenteditable><p>abc</p></div>
2. Open inspector and open context menu of the <p> in the Inspector tab
3. Select "Break on..." -> "Subtree modification"
4. Open the context menu again, then, select "Use in console"
5. Then, you'll see temp0 in the console, then, type .textContent = "xxx" and Ctrl + Enter
6. Then, you'll see pausing in the debugger
7. Select "Inspector" tab and open context menu for the <p>

What happened?

"Delete Node", "Create New Node", etc are available. And they work.

What should have happened?

I think that DOM should not be modifiable while JS is paused.

Anything else we should know?

Looks like that the break points are handled with DevTools only events like here:
https://searchfox.org/mozilla-central/rev/b8b93b11fda5fcb1f5225df7fa1e4bd261f44fea/dom/base/nsContentUtils.cpp#5843-5846

If the mutation is caused by a change by the builtin editor, the editor module cannot trust the DOM tree structure is as expected even after removing the legacy DOM mutation events. Then, the editor module cannot delete the error handling code for the unexpected mutations.

So, I'd like DevTools do not allow to modify the DOM from Inspector while the script is paused. And perhaps, Gecko itself should have a guard for this.

Smaug, WDYT?

Comment 1

[Julian Descottes [:jdescottes]]

From a web developer's point of view, being able to modify the DOM while paused is definitely useful. I remember modifying elements (changing attributes, removing nodes, adding nodes etc...) while paused to investigate issues. I would rather do it from the console than from the inspector, but still.

If the mutation is caused by a change by the builtin editor, the editor module cannot trust the DOM tree structure is as expected even after removing the legacy DOM mutation events. Then, the editor module cannot delete the error handling code for the unexpected mutations.

What is the builtin editor in this context? Do you refer to the inspector editing capabilities? Something else?
In general we would need to understand a bit better what are the potential issues linked to modifying the DOM (vs running regular scripts via the console).

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

(In reply to Julian Descottes [:jdescottes] from comment #1)

If the mutation is caused by a change by the builtin editor, the editor module cannot trust the DOM tree structure is as expected even after removing the legacy DOM mutation events. Then, the editor module cannot delete the error handling code for the unexpected mutations.

What is the builtin editor in this context? Do you refer to the inspector editing capabilities? Something else?

HTMLEditor which implements contenteditable and designMode behavior.

In general we would need to understand a bit better what are the potential issues linked to modifying the DOM (vs running regular scripts via the console).

For example, when HTMLEditor needs to split a paragraph, e.g., <div contenteditable><p><span>abc[]def</span></p></div>, HTMLEditor needs to do:

1. Create another Text
2. Create another <span>
3. Create another <p>
4. Delete "def" from the original Text
5. Insert "def" into the new Text
6. Append the new Text to the new <span>
7. Append the new <span> to the new <p>
8. Insert the new <p> after the original <p>

(Note that the order may be different actually)

In this list, #4 and #8 are trackable by observing the mutation and the #4 is a hell. The original <p> can be removed from the DOM, the contenteditable attribute of the ancestor may be remove, the <div> having contenteditable attribute may be removed, etc. So, the #8 target needs to be verified before doing that. However, all the scenarios cannot be handled actually due to crazy number of cases. Therefore, this issue has been a long standing security issue of the editor module.

Therefore, the editor module does not want nobody to intercept the DOM mutations synchronously.

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for the clarification. As I said in my previous comment, we don't want to prevent DOM manipulation when the Debugger is paused on any breakpoint. That's really a core feature of developer tools. And disabling the features in the inspector won't prevent the user from running scripts anyway, so that's not really something that can be solely handled on DevTools' side.

If the issue is constrained to mutations related to the built-in editor, could we consider one of the following:

• not breaking on those mutations - maybe with a flag to indicate unsafe mutations which devtools could check?
• or preventing DOM updates while the built in editor is still handling an update (ideally logging a clear error when doing so)

Comment 4

[Julian Descottes [:jdescottes]]

(Initial summary was apparently for Olli, so adding a ni?)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #0)

Smaug, WDYT?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Yeah, of course, we can stop dispatching the chrome only event when the editor module changes the DOM with the script blocker. So, the first option is available. However, from users of DevTools point of view, some mutations cannot be observed. That may make them report bugs.

Comment 6

[Julian Descottes [:jdescottes]]

Yes I agree we might see bugs being filed, but I think that's less likely to upset users than preventing DOM interaction on breakpoints. Ideally we would still get the mutation - with a flag - so that if the user wanted to break on subtree change or similar, we could log something explaining we can't break here due to technical limitations.

And maybe we can implement the second option after that if it is more complicated.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sick)]

Thanks, I started to write patches for bug 1973655. That will allow to the chrome event dispatchers set a new flag whether the mutation is allowed. So, we could take the second option too.