innerHTML on a script tag escapes tag characters

[jdm]

<script type="text/template"><!-- hi --></script><script>console.log(document.querySelector('script').innerHTML)</script>

In Firefox, this prints <!-- hi -->. In Servo, it prints <-- hi -->.

[jdm]

The cause is this match which says that the contents of script tags should be escaped.

[jdm]

That code matches what is written at https://html.spec.whatwg.org/multipage/syntax.html#serialising-html-fragments for "If current node is a Text node". Interestingly, outerHTML works fine, and innerHTML does not.

[jdm]

The parent() operation in the html5ever serializer gets the last element in the element stack. In the outerHTML case this is the element that is pushed in start_elem. In the innerHTML case this is the dummy initial value added in the serializer's constructor, so it's not considered to have a script element parent.

[highfive]

Please make a comment here if you intend to work on this issue. Thank you!

[jdm]

To fix this, in the case where the serializer is created with ChildrenOnly we should make the initial element contain the data for the actual element parent. We should add a unit test in html5ever for this case, as well as create a new WPT test for Servo.

[jdm]

Actually, there's a useful tests/wpt/web-platform-tests/html/syntax/serializing-html-fragments.html test that already exists, and contains a relevant case: <span><script type="test"><&></script></span>. However, it only checks innerHTML and outerHTML of the span, rather than the script directly. Let's add another case to this that gets rid of the outer span.

[tormeh]

Can I claim this? I have no experience with Servo (except testing) and little with Rust, so if it needs to be done quickly then I'm probably not the correct person...

[jdm]

Please do! There's no rush, and the actual code changes will happen in the html5ever repository.

[jdm]

To see how Servo uses the html5ever serializer, look at Element::GetInnerHTML and related code in the HTML parser serialize implementation.

[tormeh]

Hmm... This bug is tagged with "C - has test", but when I run "cargo test" all the tests pass. Is the test wrong? And the code says that the contents of a script tag should not be escaped (i.e. let script = false;), while this thread and the title says they are. What am I missing?

[jdm]

The has test label means that the issue contains a minimal code sample that reproduces the problem. It still needs to be turned into a testcase that will run as part of ./mach test-wpt (as suggested in this comment).

[jdm]

This comment explains the case in which the code does not operate correctly and chooses to escape the element contents when it should not.

[jdm]

Have you encountered any problems with fixing the issue, @tormeh?

[tormeh]

No, well, I'm new to Servo and Rust and inexperienced in HTML, git and Github. So even though the bug itself is easy, everything around it really slows me down. Which is frustrating, which slows me down further, since the world has plenty of non-frustrating things to do. But I also learn quite a bit.

[jdm]

#15148 ran into some problems due to the master of html5ever being different from the version that Servo was using. This is now available for anybody to work on again.

[seanlee31]

Hi there! I would like to work on this bug. I'm new to bugzilla/servo and may need some additional help.

I've looked into the code and wants to confirm if the main problem was caused by the return value for innerHTML from parent.

Thanks!

[seanlee31]

Also,

is the value of the node equal to <script type="text/template"><!-- hi --></script><script>console.log(document.querySelector('script').innerHTML)</script> when traversal_scope = IncludNode, and equals to <!-- hi --> when traversal_scope = ChildrenOnly?

[jdm]

That is correct.

[seanlee31]

I've read thru past comments from the previous developer but not sure where to begin with. Also, the problem was fixed and started to occur again due to the inconsistency in html5ever that were being used?

Thank you for your help!

[jdm]

No, the problem mentioned was in trying to use a local clone of html5ever in Servo, which is required in order to modify it and see the changes in Servo.

[jdm]

First: add the test that demonstrates the problem. Second: get a local clone of html5ever working. Third: fix the problem in html5ever and make the test pass.

[seanlee31]

Okay I'll add some testcases to my local servo clone just like @tormeh did. But I'm not sure how to interact the testcases in local servo clone with the local clone of html5ever you mentioned since they're two different repos?
Also, can you please assign me to this bug?
Thanks!

[jdm]

Sounds good!

[jdm]

@moonlightdrive Did you make any progress here?

[o0Ignition0o]

Heyh @jdm, sorry for the late response (I've somehow muted the repo even though i didn't want to) Please consider me if the issue gets unassigned again !

[moonlightdrive]

I added the tests and have servo building with a local clone of html5ever.

The problem I am facing now is finding where in html5ever I need to make the change. I thought the serialize function in html5ever/src/rcdom.rs seemed reasonable, but changes here have had no effect.

Additionally I am not clear on the difference between html5ever and xml5ever (or if that's even relevant to this issue)

[Ygg01]

Hi @moonlightdrive I wrote xml5ever, I'll try to help as much as possible!

Basically html5ever and xml5ever are really, really similarly looking. Think of it as twin library. I tried to make sure that things are generally located in the same space (assuming you start from project root).

For example html5ever/src/rcdom.rs, in xml5ever is located in xml5ever/src/rcdom.rs.

[jdm]

I linked to the appropriate serializer implementation in one of the earlier comments: https://github.com/servo/html5ever/blob/master/src/serialize/mod.rs

[jasonwilliams]

Looks like this is also a cause of #16532

[KiChjang]

@moonlightdrive Are you still working on this?

[moonlightdrive]

Hi, I am still working on it. Sorry I've been so slow. If this issue needs to be fixed soon I am happy to give it up and maybe take a look at another one.

[jdm]

Still making progress @moonlightdrive? Do you have any questions that we can answer?

[moonlightdrive]

I definitely have a question, or at least some confusion, but think I need to poke around a bit more to know exactly what it is.

As far as I understand, the difference between the treatment of innerHTML and outerHTML is that the former is serialized with ChildrenOnly and the latter with IncludeNode. And this bug arises due to the fact that in the ChildrenOnly case, we didn't push that script node.. so when we check the parent to see if we need to escape, we've essentially discarded that information. As has already been said.

But I don't see, in start_elem (where there are 2 instances of nodes being pushed to the stack) how the IncludeNode and ChildrenOnly cases are being treated differently.

Another question: what is happening here? Is this where script is pushed in the outerHTML case?

Not sure if I've been clear with my questions. Please let me know and I'll try to clarify.

[jdm]

@moonlightdrive I think you're looking too deeply here. start_elem is responsible for serializing a single element, and leaving a record in the stack member of the element that was just serialized. It doesn't know (or care) about whether it's invoked as part of ChildrenOnly or IncludeNode.

As to your second question, yes, through invoking start_elem there in the IncludeNode case (as used by outerHTML), the stack ends up with a meaningful entry for the outer <script> tag.

[jdm]

Looking more carefully, I think I see why you're asking those questions. I think the right thing to do might be to modify the html5ever serialize method to push a meaningful stack entry if the traversal scope is ChildrenOnly, or possibly even modify Serializer::new method to do that.

[jdm]

@moonlightdrive Are you still working on this?

[moonlightdrive]

I am actually a little unclear on where (which repository) to make this change. I have tried the following:

I tried making the fix by changing the Serializer::new method in html5ever. I am not sure how to get the tagname for the html_name field, without importing types from servo. It seems like html5ever is a more general library that should not depend on servo, correct?

Also took a look at pushing to the stack in the serialize method when the traversal scope is ChildrenOnly (implemented in servo repo). Thought I need parent method (to check if the None value should replaced by the correct thing). parent is private. I tried to make it public (would this be undesirable?) but ran into an issue of "can't leak private types" (have done some googling but am still new to rust and don't quite understand why this occurs).

Not sure if I've misunderstood how to implement this.

Thank you

[jdm]

Sorry for the delay in responding. Good investigation! You're right that html5ever cannot depend on Servo (especially since Servo depends on html5ever). The error about private types occurs because a public function would now have a private type in its public interface; the private type in this case is ElemInfo which is defined in the same file.

As for the optimal solution, I think I have a useful idea for you. Let's make the ChildrenOnly variant of TraversalScope have a Option<LocalName> field. Serializer::new can look at the traversal_scope member of the options that receives and extract the name if it's present, and use that for the initial ElemInfo value. Am I making sense?

[moonlightdrive]

Yeah, that makes sense! I'll take a look at implementing that solution. Thanks :)

[moonlightdrive]

I think what I actually need is a function that's part of the Serializer trait, which will push/pop elements to the stack, without doing any of the writes that start_elem or end_elem do.

I believe (and don't have a great grasp of Rust yet) that this function should have a signature like

// i don't know what to name this fn yet
fn start_elem_without_write<T>(&mut self, elem: T);

and that the implementation of this function for HtmlSerializer should be something like

fn start_elem_without_write<ElemInfo>(&mut self, elem: ElemInfo) {
  self.stack.push(elem);
}

but this is evidently not right. The error I am getting is: expected struct 'serialize::ElemInfo', found type parameter. I guess this type parameter is ElemInfo? (Based on a compiler note: expected type serialize::ElemInfo found type ElemInfo) I don't understand why this is a type parameter rather than a thing that is as struct.