Bug 140999 – Removing text node does not remove its associated markers

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Grzegorz Czajkowski]

Clearing text on HTMLTextAreaElement and HTMLInputElement does not remove its previous markers. 

For example, running the following script two times will cause that Document stores two markers (one for current node and one for previous which was remved!).

<script>
    textArea.value = "";
    textArea.focus();
    document.execCommand("InsertText", false, "welllcome ");
</script>

document().markers().showMarkers()

2 nodes have markers:
0x7f31b5122dc0 1:[0:9](0)
0x7f31b5122d70 1:[0:9](0)

This happens when text to insert is empty and setInnerTextValue("") calls removeChildren() on the previous node.

It does not affect user as the absolete node has been removed and its markers not drawn.
However does it make sense to store it until document's detach?

[Grzegorz Czajkowski]

Created attachment 245544 [details]
Patch

[Darin Adler]

Comment on attachment 245544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245544&action=review

> Source/WebCore/dom/ContainerNodeAlgorithms.h:242
> +    if (is<Text>(node))
> +        node.document().markers().removeMarkers(&node);

This is in a really hot code path; I don’t think we want to do this in this way. Lets find a way that is more efficient.

[Grzegorz Czajkowski]

Comment on attachment 245544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245544&action=review

>> Source/WebCore/dom/ContainerNodeAlgorithms.h:242
>> +        node.document().markers().removeMarkers(&node);
> 
> This is in a really hot code path; I don’t think we want to do this in this way. Lets find a way that is more efficient.

Right. I'll try to propose a new approach. Thanks!

[Grzegorz Czajkowski]

I was playing with this bug a bit and it turned out that it's reproducible for contenteditable as well, for example:

<script>
    div.focus();
    document.execCommand("InsertText", false, "welllcome ");

    div.innerHTML = "";   // after that we still store markers for "welllcome" in DocumentMakerController
</script>

Since the root cause is the same (we don't remove markers while children are being removed) I'll suggest removing them somewhere inside ContatinerNode::removeChildren().

[Grzegorz Czajkowski]

Created attachment 245704 [details]
test_case_1, input.value = "" does not clear markers

[Grzegorz Czajkowski]

Created attachment 245705 [details]
Patch

[Chris Dumez]

Comment on attachment 245705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245705&action=review

> Source/WebCore/dom/ContainerNode.cpp:664
> +            if (is<Text>(removedChild))

Special-casing Text here seems weird. Also, I am a bit worried about the performance impact of this change. Wouldn't it be possible to override "virtual void removedFrom(ContainerNode& insertionPoint);" in Text class and call removeMarkers() there instead? It would be much nicer if Text handling was in Text class instead of ContainerNode.

[Grzegorz Czajkowski]

Comment on attachment 245705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245705&action=review

>> Source/WebCore/dom/ContainerNode.cpp:664
>> +            if (is<Text>(removedChild))
> 
> Special-casing Text here seems weird. Also, I am a bit worried about the performance impact of this change. Wouldn't it be possible to override "virtual void removedFrom(ContainerNode& insertionPoint);" in Text class and call removeMarkers() there instead? It would be much nicer if Text handling was in Text class instead of ContainerNode.

Moving this to Text::removedFrom(ContainerNode) seems like a good idea. Thanks!
Additionally, I'll try to add some async bits to make it non-blocking.

[Grzegorz Czajkowski]

Created attachment 245871 [details]
apply Chris' comments + add async bits

[Chris Dumez]

Comment on attachment 245871 [details]
apply Chris' comments + add async bits

View in context: https://bugs.webkit.org/attachment.cgi?id=245871&action=review

> Source/WebCore/dom/DocumentMarkerController.h:81
> +    void willRemoveAllMarkersFrom(Node*);

Why doesn't this take a reference?

> Source/WebCore/dom/DocumentMarkerController.h:113
> +    void removeMarkersFromRemovedNodeTimerFired();

We usually put private methods before the private data members, not after.

> Source/WebCore/dom/Text.cpp:200
> +    document().markers().willRemoveAllMarkersFrom(this);

I am not familiar with these markers and I cannot cannot if doing things asynchronously makes sense here so I'll let someone else review. However, I'll say that willRemoveAllMarkersFrom() naming is confusing as it doesn't tell me that it actually removes the markers asynchronously. We probably want something like removeMarkersAsync().

> Source/WebCore/dom/Text.cpp:201
> +    Node::removedFrom(removalPoint);

CharacterData:: as this is the parent class. Even if CharacterData doesn't override this function now, it could later.

> Source/WebCore/dom/Text.h:53
> +    virtual void removedFrom(ContainerNode& removalPoint) override final;

Should probably be protected?

[Grzegorz Czajkowski]

(In reply to comment #10)
> Comment on attachment 245871 [details]
> apply Chris' comments + add async bits
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245871&action=review
> 
> > Source/WebCore/dom/DocumentMarkerController.h:81
> > +    void willRemoveAllMarkersFrom(Node*);
> 
> Why doesn't this take a reference?
> 
> > Source/WebCore/dom/DocumentMarkerController.h:113
> > +    void removeMarkersFromRemovedNodeTimerFired();
> 
> We usually put private methods before the private data members, not after.
> 
> > Source/WebCore/dom/Text.cpp:200
> > +    document().markers().willRemoveAllMarkersFrom(this);
> 
> I am not familiar with these markers and I cannot cannot if doing things
> asynchronously makes sense here so I'll let someone else review. However,
> I'll say that willRemoveAllMarkersFrom() naming is confusing as it doesn't
> tell me that it actually removes the markers asynchronously. We probably
> want something like removeMarkersAsync().
> 
> > Source/WebCore/dom/Text.cpp:201
> > +    Node::removedFrom(removalPoint);
> 
> CharacterData:: as this is the parent class. Even if CharacterData doesn't
> override this function now, it could later.
> 
> > Source/WebCore/dom/Text.h:53
> > +    virtual void removedFrom(ContainerNode& removalPoint) override final;
> 
> Should probably be protected?

Thanks for comprehensive review! I'll take all the comments into consideration.

[Grzegorz Czajkowski]

Created attachment 245933 [details]
Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers

[Chris Dumez]

I'll let Darin or Ryosuke review as I am not familiar with these document markers.

[Grzegorz Czajkowski]

ping reviewers
this is a friendly request for review. Thanks

[Ryosuke Niwa]

Comment on attachment 245933 [details]
Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers

View in context: https://bugs.webkit.org/attachment.cgi?id=245933&action=review

> Source/WebCore/dom/Text.cpp:203
> +void Text::removedFrom(ContainerNode& removalPoint)
> +{
> +    document().markers().removeMarkersAsync(*this);
> +    CharacterData::removedFrom(removalPoint);
> +}
> +

I don't think it makes sense to add Text::removedFrom just to remove markers.
We should be doing this in either one of Document::nodeWillBeRemoved, Document::nodeChildrenWillBeRemoved, or Document::updateRangesAfterChildrenChanged instead.

And I don't think making only this one code path async make sense. If anything, we should check hasMarkers() before calling removeMarkers
or make all of Document::textInserted, Document::textRemoved, etc… async as well.

[Ryosuke Niwa]

Comment on attachment 245933 [details]
Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers

View in context: https://bugs.webkit.org/attachment.cgi?id=245933&action=review

> Source/WebCore/dom/DocumentMarkerController.cpp:474
> +    m_removeMarkersTimerData.first = &node;
> +    m_removeMarkersTimerData.second = markerTypes;

I don't think this would work if two different nodes were removed in a single task.

[Grzegorz Czajkowski]

Created attachment 246515 [details]
test_case_2, removing text node does not clear markers

I was playing with this bug a bit and found out that we don't remove markers on div.removeChild(div.firstChild);

[Grzegorz Czajkowski]

Comment on attachment 245933 [details]
Apply Chris' comments (round 2) and let removeMarkersAync remove specific DocumentMarkers

Clear review flag due to Ryosuke's review.

[Grzegorz Czajkowski]

Created attachment 246526 [details]
Apply Ryosuke's comments

[Ryosuke Niwa]

Comment on attachment 246526 [details]
Apply Ryosuke's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=246526&action=review

> Source/WebCore/dom/Document.cpp:3713
> +    if (is<Text>(n))

We should check m_markers->hasMarkers() here as well.

[Grzegorz Czajkowski]

(In reply to comment #20)
> Comment on attachment 246526 [details]
> Apply Ryosuke's comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246526&action=review
> 
> > Source/WebCore/dom/Document.cpp:3713
> > +    if (is<Text>(n))
> 
> We should check m_markers->hasMarkers() here as well.

Happy to see r+, thanks. I think we can skip it here as inside removeMarkers() we already check them, https://trac.webkit.org/browser/trunk/Source/WebCore/dom/DocumentMarkerController.cpp#L454

[Grzegorz Czajkowski]

Committed r180139: <http://trac.webkit.org/changeset/180139>

[Radar WebKit Bug Importer]

<rdar://problem/19844628>