Closed
Bug 392511
Opened 18 years ago
Closed 18 years ago
[FIX]innerHTML getter fails to escape entities in attributes properly (so "innerHTML = innerHTML" has an unexpected side effect)
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: bugzilla, Assigned: bzbarsky)
References
Details
(Keywords: testcase)
Attachments
(2 files)
|
409 bytes,
text/html
|
Details | |
|
5.48 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
jst
:
approval1.9+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
This simple page exhibits the problem:
----
<html>
<head>
<script language="JavaScript">
<!--
function foo() {
var x = document.getElementById('sp');
x.innerHTML = x.innerHTML; // Should have no effect...
}
// -->
</script>
</head>
<body>
<span id="sp"><input type="button" value="Click me" onclick="javascript:alert('&amp;')"/></span>
<input type="button" value="Set" onclick="foo()"/>
</body>
</html>
-----
Setting the span's innerHTML to itself causes '&' inside the onclick parameter of an element inside the span to be changed to '&'.
Setting x.innerHTML = x.innerHTML is thus not side-effect free.
Reproducible: Always
Steps to Reproduce:
1. In the test page, click the "Click me" button. You should see '&'.
2. Click the "Set" button, which sets the span's innerHTML=innerHTML.
3. Now click the "Click me" button again. You now see '&'.
Actual Results:
'&' becomes '&' after setting innerHTML=innerHTML.
Expected Results:
'&' should be preserved as '&' after setting innerHTML=innerHTML.
|
||
Comment 1•18 years ago
|
||
|
|
||
Comment 2•18 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007081423 Minefield/3.0a8pre ID:2007081423
I see the same on trunk. Opera 9.22 shows & always. So does IE6
|
|
||
Updated•18 years ago
|
Version: unspecified → Trunk
|
|
||
Updated•18 years ago
|
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
|
||
Updated•18 years ago
|
Summary: setting innerHTML = innerHTML in JavaScript has side effect of HTML elements being interpreted → innerHTML getter fails to escape entities in attributes properly (so "innerHTML = innerHTML" has an unexpected side effect)
|
Assignee | |
Comment 4•18 years ago
|
||
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #279219 -
Flags: superreview?(peterv)
Attachment #279219 -
Flags: review?(peterv)
|
|
Assignee | |
Updated•18 years ago
|
Priority: -- → P2
Summary: innerHTML getter fails to escape entities in attributes properly (so "innerHTML = innerHTML" has an unexpected side effect) → [FIX]innerHTML getter fails to escape entities in attributes properly (so "innerHTML = innerHTML" has an unexpected side effect)
Target Milestone: --- → mozilla1.9 M9
|
||
Comment 5•18 years ago
|
||
Comment on attachment 279219 [details] [diff] [review]
Fix
>Index: content/base/src/nsXMLContentSerializer.cpp
>===================================================================
>@@ -560,26 +562,29 @@ nsXMLContentSerializer::SerializeAttr(co
> if (bIncludesDouble && bIncludesSingle) {
> mInAttribute = PR_TRUE;
> AppendToString(sValue, aStr, PR_FALSE);
> mInAttribute = PR_FALSE;
> }
> else {
> mInAttribute = PR_TRUE;
>- AppendToString(aValue, aStr, PR_FALSE);
>+ AppendToString(sValue, aStr, PR_FALSE);
> mInAttribute = PR_FALSE;
> }
Move those three lines after the if (combining the identical blocks from if and else).
Attachment #279219 -
Flags: superreview?(peterv)
Attachment #279219 -
Flags: superreview+
Attachment #279219 -
Flags: review?(peterv)
Attachment #279219 -
Flags: review+
|
|
Assignee | |
Comment 6•18 years ago
|
||
Comment on attachment 279219 [details] [diff] [review]
Fix
Requesting approval for HTML serialization bug that breaks round-tripping of script attributes. Risk is very low.
Attachment #279219 -
Flags: approval1.9?
|
||
Updated•18 years ago
|
Attachment #279219 -
Flags: approval1.9? → approval1.9+
|
|
Assignee | |
Comment 7•18 years ago
|
||
Checked in, with the comment 5 code change made.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
|
|
||
Updated•11 years ago
|
Blocks: innerhtml-roundtrip
|
||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•