[FIX]innerHTML getter fails to escape entities in attributes properly (so "innerHTML = innerHTML" has an unexpected side effect)

RESOLVED FIXED in mozilla1.9beta1

Status

()

P2
normal
RESOLVED FIXED
11 years ago
5 years ago

People

(Reporter: bugzilla, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla1.9beta1
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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;amp;')"/></span>
        <input type="button" value="Set" onclick="foo()"/>
    </body>
</html>
-----

Setting the span's innerHTML to itself causes '&amp;' 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 '&amp;'.
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:  
'&amp;' becomes '&' after setting innerHTML=innerHTML.

Expected Results:  
'&amp;' should be preserved as '&amp;' after setting innerHTML=innerHTML.

Updated

11 years ago
Keywords: testcase
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 &amp; always. So does IE6

Updated

11 years ago
Version: unspecified → Trunk

Updated

11 years ago
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general

Updated

11 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)

Comment 3

11 years ago
Confirmed, Mac Firefox trunk.
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 4

11 years ago
Created attachment 279219 [details] [diff] [review]
Fix
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #279219 - Flags: superreview?(peterv)
Attachment #279219 - Flags: review?(peterv)
(Assignee)

Updated

11 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 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

11 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

11 years ago
Attachment #279219 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 7

11 years ago
Checked in, with the comment 5 code change made.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 608322
You need to log in before you can comment on or make changes to this bug.