Iterative serialization of nodes #16749

ghost · 2017-05-05T23:30:03Z

This serializes the nodes in an iterative way and now it doesn't crash when serializing a deep tree

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #16696 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-05-05T23:30:08Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/servoparser/html.rs
@KiChjang: components/script/dom/servoparser/html.rs

jdm · 2017-05-10T18:07:24Z

@nox review ping!

nox

This looks correct, but the commits need to be squashed together, and I would appreciate if you would write a proper explanation in the commit, specifically how stack works, and how the children count in its items is handled.

nox · 2017-05-12T16:04:45Z

components/script/dom/servoparser/html.rs

+        if stack.is_empty() {
+            Ok(())
+        } else {
+            Err(io::Error::new(io::ErrorKind::Other, "Stack is not empty!!!!"))


What's that for? If this isn't empty, this is a logic error and should probably panic instead.

nox · 2017-05-12T16:04:45Z

components/script/dom/servoparser/html.rs

+            roots.next().unwrap();
+        }
+        'traversal: for root in roots {
+            let node = root.r();


I don't think you need this here, just do for node in roots.

nox · 2017-05-12T16:04:45Z

components/script/dom/servoparser/html.rs

+        }
+        'traversal: for root in roots {
+            let node = root.r();
+            match  node.type_id() {


Nit: there is one space too many here, after match.


        Now a node is serialized in an iterative way. For this, we iterate

over the tree generated by the node in preorder using a vector as a stack. When we call the serializer's method start_elem, we add a tuple with the node's name and the total number of children. We substract one from the number of children of the last element of the vector when we serialize a node or when we call the serializer's method end_elem. The purpose of this stack is to take into account which nodes need to be ended by the serializer and when they need to be ended. If the last element of the vector has no children left, it is removed from the vector and ended by the serializer using end_elem. Another way to think about this, is that the last element of the vector always contains the name of the parent of the current node during the iteration over the tree.

ghost · 2017-05-13T06:48:26Z

OK @nox I believe it's ready

nox · 2017-05-15T11:39:20Z

@bors-servo r+

bors-servo · 2017-05-15T11:39:20Z

📌 Commit f61c07f has been approved by nox

bors-servo · 2017-05-15T12:04:18Z

⌛ Testing commit f61c07f with merge e175826...


        Auto merge of #16749 - z1mvader:serialize_nodes_preorder, r=nox

Iterative serialization of nodes  This serializes the nodes in an iterative way and now it doesn't crash when serializing a deep tree ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #16696 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16749)

bors-servo · 2017-05-15T12:32:08Z

💔 Test failed - linux-rel-wpt

nox · 2017-05-15T12:57:40Z

  ▶ Unexpected subtest result in /html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:
  │ FAIL [expected PASS] Template contents should be serialized instead of template element if serializing template element. Test serializing whole document
  │   → assert_equals: template element is serialized incorrectly expected "<html><head><title>Test Document</title></head><body><template><div id=\"div1\">some text</div></template></body></html>" but got "<html><head><title>Test Document</title></head><body><template></template></body></html>"
  │ 
  │ @http://web-platform.test:8000/html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:62:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:52:1

  ▶ Unexpected subtest result in /html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:
  │ FAIL [expected PASS] Template contents should be serialized instead of template element if serializing template element. Test nested template
  │   → assert_equals: template element is serialized incorrectly expected "<template><template><div id=\"div1\">some text</div></template></template>" but got "<template></template>"
  │ 
  │ @http://web-platform.test:8000/html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:44:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:32:1

  ▶ Unexpected subtest result in /html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:
  │ FAIL [expected PASS] Template contents should be serialized instead of template element if serializing template element
  │   → assert_equals: template element is serialized incorrectly expected "<template><div id=\"div1\">some text</div></template>" but got "<template></template>"
  │ 
  │ @http://web-platform.test:8000/html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:25:5
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  │ test@http://web-platform.test:8000/resources/testharness.js:497:9
  └ @http://web-platform.test:8000/html/semantics/scripting-1/the-template-element/serializing-html-templates/outerhtml.html:16:1

You broke serialization of templates.

ghost · 2017-05-15T14:13:51Z

Ok the problem is that I didn't include this part: https://github.com/servo/servo/blob/master/components/script/dom/servoparser/html.rs#L141

The thing is, We are traversing the tree in preorder and then we don't know which nodes are the children of the current node. should I try to downcast to HTMLTemplateElement and upcast to Node every node?

nox · 2017-05-15T14:31:28Z

I would make stack a Vec<(Root<Node>, usize)>, and store in that the DocumentFragment associated with the template, instead of the template itself. Then when it's time to print the end tag of these nodes, write </template> if the node is a DocumentFragment.

nox · 2017-05-15T14:32:26Z

Oh never mind, I see what the problem is. You probably want to make a new iterator, somehow.

ghost · 2017-05-15T15:33:17Z

could you tell me what do you have in mind?

nox · 2017-05-15T15:35:00Z

You need a new iterator, that does exactly the same traversal as the one returned by traverse_preorder, but which traverse the template's contents instead of its children.

highfive assigned nox May 5, 2017

highfive added the S-awaiting-review label May 5, 2017

ghost mentioned this pull request May 10, 2017

Node rendering can trigger segmentation fault #16776

Open

nox added the S-needs-squash label May 12, 2017

nox requested changes May 12, 2017

View changes

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels May 15, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels May 15, 2017

nox approved these changes May 15, 2017

View changes

nox added S-needs-code-changes and removed S-tests-failed labels May 15, 2017

ghost closed this Jun 13, 2017

jdm mentioned this pull request Jun 21, 2017

Node serialization can trigger stack overflow #16696

Closed

ghost deleted the serialize_nodes_preorder branch Jun 21, 2017

ghost restored the serialize_nodes_preorder branch Jun 21, 2017

This issue was closed.

servo / servo

Iterative serialization of nodes #16749

Iterative serialization of nodes #16749

ghost commented May 5, 2017 •

edited by ghost

highfive commented May 5, 2017

jdm commented May 10, 2017

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ghost commented May 13, 2017

nox commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

nox commented May 15, 2017

ghost commented May 15, 2017

nox commented May 15, 2017

nox commented May 15, 2017

ghost commented May 15, 2017

nox commented May 15, 2017

servo / servo

Join GitHub today

Iterative serialization of nodes #16749

Iterative serialization of nodes #16749

Conversation

ghost commented May 5, 2017 • edited by ghost

This serializes the nodes in an iterative way and now it doesn't crash when serializing a deep tree

highfive commented May 5, 2017

jdm commented May 10, 2017

nox left a comment

This comment has been minimized.

nox May 12, 2017

This comment has been minimized.

ghost May 13, 2017

This comment has been minimized.

nox May 12, 2017

This comment has been minimized.

ghost May 13, 2017

This comment has been minimized.

nox May 12, 2017

This comment has been minimized.

ghost May 13, 2017

ghost commented May 13, 2017

nox commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

nox commented May 15, 2017

ghost commented May 15, 2017

nox commented May 15, 2017

nox commented May 15, 2017

ghost commented May 15, 2017

nox commented May 15, 2017

ghost commented May 5, 2017 •

edited by ghost