Can we notify DevTools of node removals before touching the DOM when removing multiple nodes?
Categories
(Core :: DOM: Core & HTML, enhancement)
Tracking
()
People
(Reporter: masayuki, Unassigned)
References
Details
(Whiteboard: [devtools-triage])
For example, nsRange::CutContents which is an implementation of Range.extractContents and Selection.deleteFromDocument deletes multiple nodes entirely in the range. Once dispatching an event synchronously for DevTools, anything could happen. Therefore, such loops need to validate some things which are required to continue the further processing.
Making this safer and reducing the number of running the validation, I wonder if we can notify DevTools of all node removals first. Then, start removing them from the DOM without notifying DevTools again. However, there are some concerns.
First, the user using DevTools will see different DOM tree when observing 2nd or latter node's removal since all nodes are still at the original position.
Next, the user may not receive node removals if there are subdocuments and beforeunload or unload event listener modifies the nodes in the document of the removing node. (If it's cheaper than the validation, we could do that only when there is no subdocuments in the range.)
Julian, WDYT about this? It's fine to mark this bug as invalid, but I'd like to know what's the important things of the feature and the knowledge will help the DOM team to maintain the DOM mutations. Thanks.
|
||
Comment 1•15 days ago
|
||
My feeling is that the main value from the "break on" features is to identify the script which is triggering the removal, rather than trying to mess with the page's state while you are paused. So I think we have some flexibility here. That being said, if the state is really surprising, users might get confused so we need to balance the two.
First, the user using DevTools will see different DOM tree when observing 2nd or latter node's removal since all nodes are still at the original position.
Not sure I understand correctly. If I am breaking on node removal for 2 nodes which are going to be deleted at the same time, what will be the state for the first pause and for the second pause? From the initial description I thought that since you want to notify devtools of all removals at first we would see the same state in all pauses, but the sentence above says the opposite.
Next, the user may not receive node removals if there are subdocuments and beforeunload or unload event listener modifies the nodes in the document of the removing node. (If it's cheaper than the validation, we could do that only when there is no subdocuments in the range.)
This is a bigger issue. I think we should try to trigger the breakpoint in all cases. It's a bit difficult to communicate to the user that a node removal might be missed.
If the concern is performance related (and not architecture/code complexity), an option would be to only do this when DevTools are watching the context? https://searchfox.org/firefox-main/rev/7032b912c282da6b9b8219e779b2a441a27aaec9/dom/chrome-webidl/BrowsingContext.webidl#218
[SetterThrows] attribute boolean watchedByDevTools;
Back to you to check my questions, I will bring this to the team anyway later today.
|
Reporter | |
Comment 2•15 days ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #1)
My feeling is that the main value from the "break on" features is to identify the script which is triggering the removal, rather than trying to mess with the page's state while you are paused. So I think we have some flexibility here. That being said, if the state is really surprising, users might get confused so we need to balance the two.
Thank you, the explanation about the basic concept is really helpful to understand.
First, the user using DevTools will see different DOM tree when observing 2nd or latter node's removal since all nodes are still at the original position.
Not sure I understand correctly. If I am breaking on node removal for 2 nodes which are going to be deleted at the same time, what will be the state for the first pause and for the second pause? From the initial description I thought that since you want to notify devtools of all removals at first we would see the same state in all pauses, but the sentence above says the opposite.
I meant that users will see the same DOM tree of any break of a set of removals but currently, they see exactly matching DOM tree from the removal time. So, the users will see different DOM tree from current result if we'd change the behavior.
Next, the user may not receive node removals if there are subdocuments and beforeunload or unload event listener modifies the nodes in the document of the removing node. (If it's cheaper than the validation, we could do that only when there is no subdocuments in the range.)
This is a bigger issue. I think we should try to trigger the breakpoint in all cases. It's a bit difficult to communicate to the user that a node removal might be missed.
Yeah, I realized that after I tried to ask you about the first case. We won't change the behavior without fixing this issue.
If the concern is performance related (and not architecture/code complexity), an option would be to only do this when DevTools are watching the context? https://searchfox.org/firefox-main/rev/7032b912c282da6b9b8219e779b2a441a27aaec9/dom/chrome-webidl/BrowsingContext.webidl#218
Yeah, one thing is about the performance, but the other is for reducing the tons of (incomplete) validation of the DOM after every mutation.
|
|
||
Updated•12 days ago
|
|
|
||
Comment 3•6 days ago
|
||
Would be useful to get an example of how the tree will look like when pausing on a breakpoint.
|
||
Comment 4•6 days ago
|
||
I think it would be fine to have an "about to be removed" node list. But It isn't clear why we would miss the node removal because of such change? Nor why we would miss notification on unload/beforeunload. Do we stop emitting mutations events to DevTools on page unload? Otherwise, this sync notification should reach devtools and prevent further destructions.
Is this about removing validation done by ValidateCurrentNode?
I imagine the change would be about dispatching a node list before trigerring the CutContents logic... but you would still need to validate that all dom elements of the notified list still exists before running CutContents logic? But you would do it only once and not spread this across the codebase.
The only scenario I could think about is if DevTools, when pausing on node removal, adds more children elements into the to-be-removed container and set breakpoints on these new elements... then yes, we won't have further mutation notifications for their removal. If that's only about that... it sounds fine not supporting such edgecase.
Also, I'm wondering about the usage of AppendChild in CutContents. Why would we append elements when cutting some? Would you also notify about these mutation additions beforehand?
|
|
||
Comment 5•6 days ago
|
||
Masayuki, can you check the comment from :ochameau above?
On my side, I want to confirm the state of the tree devtools should see in the notification: will all the nodes be already removed? some? none? Thanks!
|
|
Reporter | |
Comment 6•5 days ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #3)
Would be useful to get an example of how the tree will look like when pausing on a breakpoint.
(I assume that DevTools synchronously block the modifying the DOM stack.)
When breaking on the removal and if Gecko dispatches devtoolschildremoved events before touching the DOM, the DOM tree which the user shows in the inspector will be the same as the call of API which removes multiple nodes. I.e., if it's Range.extractContents, the DOM tree has not been changed by the call even though the watching node is removed second or later so that the node which should've already been removed when the watching node is removed are still connected to the DOM tree.
(In reply to Alexandre Poirot [:ochameau] from comment #4)
I think it would be fine to have an "about to be removed" node list. But It isn't clear why we would miss the node removal because of such change? Nor why we would miss notification on unload/beforeunload. Do we stop emitting mutations events to DevTools on page unload? Otherwise, this sync notification should reach devtools and prevent further destructions.
I meant that unload and beforeunload event of subdocumens contained in a removal node may touch the DOM around removing node. Therefore, if Gecko always notifies DevTools of removing node list first, the notified node might not be removed from the DOM later. (Oh, but this might be impossible case unless there is an error handling path in the loop and the beforeunload and unload event listener causes the error...)
It seems that, anyway, the removing node list contains subdocuments, Gecko should not notify DevTools of the removing node list. In that case, Gecko anyway needs to validate the range after each node removal.
Is this about removing validation done by ValidateCurrentNode?
I imagine the change would be about dispatching a node list before trigerring the CutContents logic... but you would still need to validate that all dom elements of the notified list still exists before running CutContents logic? But you would do it only once and not spread this across the codebase.
Yes, I intent to remove that as far as possible because of the running cost (not the maintenance cost). However, note that similar issues are in the editor modules in the various scenarios. My main purpose of this asking is I'd like to know what I can do in the editor module.
I think that if we can notify DevTools of the list, the following code can just remove the nodes in the list. That might get unexpected result for the running script, but it's caused by the user's interception. So, the behavior must be acceptable.
The only scenario I could think about is if DevTools, when pausing on node removal, adds more children elements into the to-be-removed container and set breakpoints on these new elements... then yes, we won't have further mutation notifications for their removal. If that's only about that... it sounds fine not supporting such edgecase.
Oh, thanks, I didn't realize the scenario. Synchronous blocking feature makes the DOM tree modifiers weaker from security and stability points of view. Most of the former point has gone by removing the legacy DOM mutation observer. However, the latter is still a real issue to avoid crash, etc. Therefore, I'd like to reduce the runtime cost and the maintenance cost of validations after each mutation as far as possible.
Also, I'm wondering about the usage of AppendChild in CutContents. Why would we append elements when cutting some? Would you also notify about these mutation additions beforehand?
The appending nodes are into a new DocumentFragment for making the result of Range.extractContents). Therefore, I think inserting nodes cannot be tracked by DevTools because of disconnected node event from the document. (Although DevTools user can restore it into the document with script if they store the node to a variable and paused on during the removals.)
(In reply to Julian Descottes [:jdescottes] from comment #5)
On my side, I want to confirm the state of the tree devtools should see in the notification: will all the nodes be already removed? some? none? Thanks!
If Gecko would notify all the node removals first, then, nothing have not been reported in the simple case. However, if we'd do this in the editor module, DevTools might be notified multiple times if the editor tries to delete multiple nodes in separated methods.
FYI: Similar thing has already done by nsINode::ReplaceOrInsertBefore() only when replaceChild is called with a connected node in a document. DevTools should be notified of the inserting node removal from the original position after the replaced node is removed if we strictly use the standardized steps (between step 11 and 13).
https://searchfox.org/firefox-main/rev/54da8f6bfead7871ca89f2cb18323af5f00d9620/dom/base/nsINode.cpp#2836-2853
Description
•