1170334 - nsWebBrowserPersist unescapes (& doesn't re-escape) XML stylesheet processing instructions

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

nsWebBrowserPersist::FixupXMLStyleSheetLink adjusts an XML document's <?xml-stylesheet?> directive to change the "href" pseudo-parameter to point to the downloaded file; to do this, it has to parse and reconstruct the data string.  In the beginning (bug 136718, 2002), it used an ad-hoc parser that didn't unescape entities; that was later extracted into nsParserUtils, and in bug 286132 (2006) the parser was changed to do entity unescaping… but nsWebBrowserPersist wasn't changed to re-escape the result.

I have a mochitest-chrome test that reproduces the bug; I don't have an actual fix yet, but it should be relatively simple.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

Attachment: Patch that adds (currently failing) test.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

…and now I have a patch that seems to work (try run pending), so I might as well just take the bug.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

Attachment: Patch [v1]

This patch adds a simple implementation of the necessary entity escaping, because there didn't seem to be one anywhere else in the tree that could be used from here.  No attempt is made to optimize the common case of literal characters, because I'm assuming that this is not performance-critical.  It might be nice if there were a more elegant way to do this.

(It would also be possible to fix the violations of today's code style, factor things more and not repeat the nsGkAtoms strings, etc.; but I don't know if this code is worth the effort.)

Comment 4

[Olli Pettay [:smaug]]

Comment on attachment 8614859 [details] [diff] [review]
Patch [v1]

Since this is parsing/serialization related bug, hsivonen should review this
(in other words, I'm trying to reduce my reviewing overload).

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

(In reply to Olli Pettay [:smaug] (Reviewing overload, no new review requests before June 7, please) from comment #4)
> Since this is parsing/serialization related bug, hsivonen should review this
> (in other words, I'm trying to reduce my reviewing overload).

Thanks for the pointer; trying to figure out the right reviewer(s) for old code like this can be difficult.

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 8614859 [details] [diff] [review]
Patch [v1]

Review of attachment 8614859 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp
@@ +2538,5 @@
> +    aBuffer.Append(key);
> +    aBuffer.AppendLiteral("=\"");
> +    for (size_t i = 0; i < aValue.Length(); ++i) {
> +        switch (aValue[i]) {
> +        case '&': aBuffer.AppendLiteral("&amp;"); break;

Please indent these as:

switch(aValue[i]) {
    case '&':
        aBuffer.AppendLiteral("&amp;");
        break;

to conform to the code style. r+ with that.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

Attachment: Patch [v2]

Reformatted as requested; carrying over r+.  Thanks for the review.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧]

This Try run is mostly done, and the orange looks like not my fault: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb4fccfb311e

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97469b491868

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97469b491868