Closed Bug 392511 Opened 17 years ago Closed 17 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)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta1

People

(Reporter: bugzilla, Assigned: bzbarsky)

References

Details

(Keywords: testcase)

Attachments

(2 files)

testcase from comment 0
409 bytes, text/html
Details
Fix
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;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.
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
Version: unspecified → Trunk
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
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)
Confirmed, Mac Firefox trunk.
OS: Windows XP → All
Hardware: PC → All
Attached patch Fix — — Splinter Review 
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

        Attachment #279219 -
        Flags: superreview?(peterv)

        Attachment #279219 -
        Flags: review?(peterv)

    
  
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+

    
    

    
  
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?

        Attachment #279219 -
        Flags: approval1.9? → approval1.9+

    
    

    
  
Checked in, with the comment 5 code change made.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: