Bug 43579 – js-test-pre.js's escapeHTML should escape null characters so we can see them

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

js-test-pre.js's escapeHTML should escape null characters so we can see them

[Adam Barth]

Created attachment 63637 [details]
Patch

[Darin Adler]

Comment on attachment 63637 [details]
Patch

> -    return text.replace(/&/g, "&amp;").replace(/</g, "&lt;");
> +    return text.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/\0/g, "\\0");

I was thinking we should chose a form that can be parsed such as &#0; but perhaps there is no such thing.

r=me

[Adam Barth]

> I was thinking we should chose a form that can be parsed such as � but perhaps there is no such thing.

The problem is that will get decoded by the HTML parser and turned into a null character again.  :)

[Adam Barth]

Comment on attachment 63637 [details]
Patch

Clearing flags on attachment: 63637

Committed r64796: <http://trac.webkit.org/changeset/64796>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #3)
> The problem is that will get decoded by the HTML parser and turned into a null character again.  :)

Are you sure? I don’t think it will.

[Adam Barth]

> Are you sure? I don’t think it will.

http://trac.webkit.org/browser/trunk/WebCore/html/HTMLEntityParser.cpp#L66

Looks like the entity parser itself catches null and replaces it with U+FFFD.

[Darin Adler]

The only downside with turning a null character into "\0" is that the characters "\0" also display as "\0". I guess I can’t think of any better thing to do, though, so I’m not sure what my point is!

[Adam Barth]

> The only downside with turning a null character into "\0" is that the characters "\0" also display as "\0".

That seems like a bug too, though.  Isn't the output of these tests meant to look like a JavaScript string?

[Darin Adler]

(In reply to comment #9)
> That seems like a bug too, though.  Isn't the output of these tests meant to look like a JavaScript string?

On reflection, I erred in my original suggestion.

The job of escapeHTML is to take a string and put it in a format where if it’s parsed as HTML you get the original string back. And it can almost do that but not quite, because of the special rule about null characters that makes it impossible in the general case. So I was wrong to suggest adding special handling of null characters to that function without renaming it. Tests should not need to serialize and then re-parse; fixing that would eliminate the need for escapeHTML entirely.

Escaping for human readability is another type of function. I don't think the test machinery tries to do that at all, but it could. If there was a function that tried to do that, it should handle more control characters than just the null character, and possibly many other non-ASCII characters as well.

That’s all from memory without studying js-test-pre.js further. I might have other ideas if I read it over.

[Adam Barth]

Would you like me to revert this patch while we consider further?

[Eric Seidel (no email)]

IIRC, ggaren is the (or at least one of the) original author(s) of js-test-pre.js

[Darin Adler]

There’s no need to revert the change. But we can come up with something better for the future.

For this discussion it doesn’t matter who the original author of js-test-pre.js is. I’m pretty sure it was Maciej, though.