Closed
Bug 1170334
Opened 10 years ago
Closed 10 years ago
nsWebBrowserPersist unescapes (& doesn't re-escape) XML stylesheet processing instructions
Categories
(Core Graveyard :: Embedding: APIs, defect)
Core Graveyard
Embedding: APIs
Tracking
(firefox41 fixed)
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jld, Assigned: jld)
Details
Attachments
(1 file, 2 obsolete files)
7.63 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
…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
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
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("&"); break;
Please indent these as:
switch(aValue[i]) {
case '&':
aBuffer.AppendLiteral("&");
break;
to conform to the code style. r+ with that.
Attachment #8614859 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Reformatted as requested; carrying over r+. Thanks for the review.
Attachment #8614859 -
Attachment is obsolete: true
Attachment #8620739 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
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
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•