Closed Bug 1170334 Opened 5 years ago Closed 5 years ago

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

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jld, Assigned: jld)

Details

Attachments

(1 file, 2 obsolete files)

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.
…and now I have a patch that seems to work (try run pending), so I might as well just take the bug.
Assignee: nobody → jld
Attached patch Patch [v1] (obsolete) — Splinter Review
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.)
Attachment #8613741 - Attachment is obsolete: true
Attachment #8614859 - Flags: review?(bugs)
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).
Attachment #8614859 - Flags: review?(bugs) → review?(hsivonen)
(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 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.
Attachment #8614859 - Flags: review?(hsivonen) → review+
Attached patch Patch [v2]Splinter Review
Reformatted as requested; carrying over r+.  Thanks for the review.
Attachment #8614859 - Attachment is obsolete: true
Attachment #8620739 - Flags: review+
This Try run is mostly done, and the orange looks like not my fault: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb4fccfb311e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/97469b491868
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.