Bug 17125 – Acid3 requires Text.replaceWholeText method from DOM Level 3

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

[Eric Seidel (no email)]

Acid3 requires Text.wholeText method from DOM Level 3

Hyatt said that the :empty test depends on this (and once he lands his :empty) fix we'll start failing as a result.

Looks easy enough to implement.

[Dave Hyatt]

It actually requires .replaceWholeText.  I'd expect a patch to implement both methods.

[Eric Seidel (no email)]

Created attachment 18899 [details]
First pass at wholeText/replaceWholeText support

 LayoutTests/ChangeLog                              |   21 +++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   17 ++++
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   80 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 17 files changed, 138 insertions(+), 21 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 18899 [details]
First pass at wholeText/replaceWholeText support

Maciej has convinced me to improve handling of EntityReferences as part of this patch (and add some test cases to make sure we make them ReadOnly nodes).  I'm gone until Wed. night, at which point I'll look at finishing this.

[Eric Seidel (no email)]

Created attachment 18955 [details]
Updated after Maciej's comments

 LayoutTests/ChangeLog                              |   27 ++++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../readonly-exceptions-expected.txt               |   23 +++++
 .../dom/EntityReference/readonly-exceptions.html   |   13 +++
 .../dom/EntityReference/resources/TEMPLATE.html    |   13 +++
 .../resources/readonly-exceptions.js               |   33 +++++++
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   23 +++++
 WebCore/dom/Document.cpp                           |    7 ++-
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   89 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 22 files changed, 247 insertions(+), 22 deletions(-)

[Darin Adler]

Comment on attachment 18955 [details]
Updated after Maciej's comments

+        if (n->nodeType() == Node::ENTITY_REFERENCE_NODE) {
+            // We would need to visit EntityReference child text nodes if they existed
+            ASSERT(!n->hasChildNodes());
+            break;
+        }
+        if (!n->isTextNode())
+            break;

The code above is (slightly) unnecessarily inefficient. The code to handle EntityReference could simply be an assertion. And if not, and you want to retain the nodeType check, then it's better to not call both nodeType() and isTextNode(). If you have the node type, you can check if something's a text node without a second virtual function call.

Because DOM mutation events, specifically the DOMNodeRemovedEvent, can do arbitrary operations, the replaceWholeText function needs some work to be robust in the face of event handler functions that do strange things.

+    for (Node* n = startText; n != this;) {

This loop variable needs to be a RefPtr. The removeChild function could remove the last reference to the parent, so the node "n" could be deleted unless someone has ref'd it.

Similarly, the current node ("this") needs to go into a RefPtr before calling functions that can in turn invoke mutation event handlers.

+        parentNode()->removeChild(nodeToRemove, ignored);
+        ASSERT(!ignored);

This assertion is incorrect. For example, the mutation function could remove the node inside the function, which will result in a NOT_FOUND_ERR. So it may be OK to ignore the error, but it's not OK to assert there was no error.

+        Node* onePastEndText = endText->nextSibling();
+        for (Node* n = nextSibling(); n != onePastEndText;) {

Same thing here. Both onePastEndText and n need to be RefPtr.

+            parentNode()->removeChild(nodeToRemove, ignored);
+            ASSERT(!ignored);

And this assertion is also incorrect.

+    setData(newText, ignored);
+    ASSERT(!ignored);

To make this assertion correct, you'll need to reset "ignored" to 0 just before calling setData since the earlier functions could have changed ignored to a non-0 value. Or you could remove the assertion. If you removed all the assertions, then you could leave ignored uninitialized.

+    Text* replaceWholeText(const String&, ExceptionCode&);

The return value of the function needs to be a PassRefPtr. It's possible that mutation event handlers will remove the text node from tree. If they do, it's important that we don't return a deleted object. The usual way we fix this elsewhere is to use PassRefPtr even though in the normal case the object will be in the DOM tree. This is why the return value from Node::appendChild, for example, is PassRefPtr and not a raw pointer.

+    String wholeText();

Could we make this a const member?

+shouldBe("entityReference.textContent", "''"); // change to '<' when we support EntityReference nodes
+shouldBe("childrenBeforeFailedAppend", "0"); // Change to 1 if Entity node support is added.
+shouldBe("childrenBeforeFailedAppend", "0"); // Change to 1 if Entity node support is added.

I'd prefer that the shouldBe results reflect correct behavior, rather than the current behavior of our engine. The expected test results will still regression-test our current behavior, because FAIL and the current values will be in the test output.

[Eric Seidel (no email)]

+        Node* onePastEndText = endText->nextSibling();
+        for (Node* n = nextSibling(); n != onePastEndText;) {

Same thing here. Both onePastEndText and n need to be RefPtr.

If we're protecting ourselves from insane mutation event handlers... couldn't a mutation event handler end up removing onePastEndText from the sibling list?  Thus if onePastEndText != 0, we'd crash, no?

[Eric Seidel (no email)]

Bah!  This is insane.  Mutation event handlers could do all sorts of crazy things, including transplanting the node we're next going to traverse to somewhere else in the document.  Thus making it impossible to simply traverse the sibling list.  We'd end up just removing random nodes until we walked off the end of whatever node-list we got transplanted into!

[Eric Seidel (no email)]

CCing darin so he can see my comments about the insanity that is trying to protect ourselves from rogue mutation event handlers.

[Eric Seidel (no email)]

I originally had wholeText() const, but thought all the const and const_cast crud was ugly.  I added it back in my forthcoming patch however.

[Eric Seidel (no email)]

Created attachment 18996 [details]
Address Darin's concerns (delta from previous patch)

 .../readonly-exceptions-expected.txt               |    6 +-
 .../resources/readonly-exceptions.js               |    6 +-
 WebCore/dom/Text.cpp                               |   70 +++++++++++---------
 WebCore/dom/Text.h                                 |    4 +-
 4 files changed, 46 insertions(+), 40 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 18996 [details]
Address Darin's concerns (delta from previous patch)

I can upload a combined patch if you need.

[Eric Seidel (no email)]

Created attachment 18997 [details]
Address Darin's concerns (delta from previous patch)

 .../readonly-exceptions-expected.txt               |    6 +-
 .../resources/readonly-exceptions.js               |    6 +-
 WebCore/dom/Text.cpp                               |   70 +++++++++++---------
 WebCore/dom/Text.h                                 |    4 +-
 4 files changed, 46 insertions(+), 40 deletions(-)

[Darin Adler]

(In reply to comment #6)
> If we're protecting ourselves from insane mutation event handlers... couldn't a
> mutation event handler end up removing onePastEndText from the sibling list? 
> Thus if onePastEndText != 0, we'd crash, no?

Our goal is to protect ourselves well enough from insane event handlers to prevent them from causing a crash or infinite loop. That may be an impossible challenge though. We should at least do the basics for now and think more deeply about how to handle the tougher issues.

[Darin Adler]

Comment on attachment 18997 [details]
Address Darin's concerns (delta from previous patch)

I would have preferred to review a combined patch:

+        case Node::CDATA_SECTION_NODE:
+            t = static_cast<const Text*>(n);

The above is incorrect. A CDATA section will be a CharacterData node and *not* a Text node. So it's an error to cast to Text*. Text is derived from CharacterData, so if we want to handle both we should be using the type CharacterData everywhere, and not Text.

I feel bad for spreading the const disease into this code with my suggestion of marking wholeText() const. Sorry. I probably would have put the const_cast calls at the call sites inside Text::wholeText and left the helper functions themselves alone.

+            RefPtr<Node> nodeToRemove(n.release());
             n = n->nextSibling();

This doesn't look like it will work. Calling release() will set n to 0 and you'll crash on the n->nextSibling() call. You should re-run the tests.

[Eric Seidel (no email)]

Yes, the RefPtr nodeToRemove(n.release()) was bogus.  I'll fix.

I think you're confused about CDATA_SECTION_NODE.  That means its a CDataSection, which is a subclass of Text (which is a subclass of CharacterData).

class CDATASection : public Text

[Darin Adler]

(In reply to comment #15)
> I think you're confused about CDATA_SECTION_NODE.  That means its a
> CDataSection, which is a subclass of Text (which is a subclass of
> CharacterData).
> 
> class CDATASection : public Text

Got it. My bad.

[Eric Seidel (no email)]

Created attachment 18999 [details]
Add wholeText/replaceWholeText support (final squashed patch, tests all pass)

 LayoutTests/ChangeLog                              |   27 ++++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../readonly-exceptions-expected.txt               |   23 +++++
 .../dom/EntityReference/readonly-exceptions.html   |   13 +++
 .../dom/EntityReference/resources/TEMPLATE.html    |   13 +++
 .../resources/readonly-exceptions.js               |   33 +++++++
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   23 +++++
 WebCore/dom/Document.cpp                           |    7 ++-
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   95 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 22 files changed, 253 insertions(+), 22 deletions(-)

[Eric Seidel (no email)]

Created attachment 19000 [details]
Add wholeText/replaceWholeText support (final squashed patch, tests all pass)

 LayoutTests/ChangeLog                              |   33 +++++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../readonly-exceptions-expected.txt               |   23 +++++
 .../dom/EntityReference/readonly-exceptions.html   |   13 +++
 .../dom/EntityReference/resources/TEMPLATE.html    |   13 +++
 .../resources/readonly-exceptions.js               |   33 +++++++
 .../fast/dom/Text/replaceWholeText-expected.txt    |   13 +++
 LayoutTests/fast/dom/Text/replaceWholeText.html    |   13 +++
 LayoutTests/fast/dom/Text/resources/TEMPLATE.html  |   13 +++
 .../fast/dom/Text/resources/replaceWholeText.js    |   16 ++++
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   23 +++++
 WebCore/dom/Document.cpp                           |    7 ++-
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   91 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 26 files changed, 310 insertions(+), 22 deletions(-)

[Darin Adler]

Comment on attachment 19000 [details]
Add wholeText/replaceWholeText support (final squashed patch, tests all pass)

r=me

[Eric Seidel (no email)]

Landed in r30088.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.