Closed Bug 123899 Opened 23 years ago Closed 22 years ago

document.body.innerHTML is broken

Categories

(Core :: DOM: Core & HTML, defect, P2)

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

Tracking

()

Status:
RESOLVED WORKSFORME
Milestone:
mozilla1.3alpha

People

(Reporter: giscardg, Assigned: peterv)

References

Details

Attachments

(4 files, 1 obsolete file)

Fix v2
1.00 KB, patch
fabian
: review+
jst
: superreview+
Details | Diff | Splinter Review
simple testcase for the document.write() of an IMG into a document.
651 bytes, text/html
Details
testcase for innerHTML
803 bytes, text/html
Details
Minimized testcase
197 bytes, text/html
Details 
<body>
</body>
<script>alert(document.body.innerHTML)</script>

The following alerts "<script>alert(document.body.innerHTML)</script>"

<body>
</body>
<script>document.body.innerHTML="hello";</script>

The following would set the body content to "hellohello"
<script> is not allowed as an immediate child of of the <html> tag, so mozilla
cleans that up and puts the script element inside the body here. However, the
fact that document.body.innerHTML="hello" ends up duplicating the word "hello"
in the presentation is a valid bug. That also happens if you put the script
inside the body tag where it belongs.

Peter, wanto have a look at this? It seems like the content sink ends up
notifying layout about the text "hello" twice here so we end up creating two
text frames for it, the content model correctly contains only "hello", not
"hellohello". If you won't have time to look into this soon, hand it to harishd
or myself.
Assignee: jst → peterv
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Priority: -- → P3
Attached patch Fix v1 (obsolete) — — Splinter Review 
This fixes the issue. We need to make sure the content sink updates its context
after script evaluation, since that might have added content and the document
already got notified about that content.
BTW, it's not clear from the patch but the added lines are in
HTMLContentSink::PostEvaluateScript.
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: --- → mozilla1.0
Comment on attachment 73771 [details] [diff] [review]
Fix v1

Makes a lot of sense to me. I'm amazed that this hasn't shown up more
frequently though. Weird.

sr=jst
Attachment #73771 - Flags: superreview+
Comment on attachment 73771 [details] [diff] [review]
Fix v1

r=fabian
Attachment #73771 - Flags: review+
Comment on attachment 73771 [details] [diff] [review]
Fix v1

This one is not good, now we're dropping some content because PreEvaluateScript
doesn't flush with notifications.
Attachment #73771 - Flags: needs-work+
Attached patch Fix v2 — — Splinter Review 
This one uses FlushPendingNotifications in PreEvaluateScript. All the missing
content that I saw seem to have been solved, and the double content is also
solved. Running pageloader (over a slow modem) gives
With patch: 6507, 1442, 26395, 6714
Without patch: 6551, 1488, 20607, 6635

Looking for reviews again.
Attachment #73771 - Attachment is obsolete: true
Comment on attachment 74520 [details] [diff] [review]
Fix v2

sr=jst
Attachment #74520 - Flags: superreview+
Comment on attachment 74520 [details] [diff] [review]
Fix v2

r=fabian
Attachment #74520 - Flags: review+
Comment on attachment 74520 [details] [diff] [review]
Fix v2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74520 - Flags: approval+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
It looks like this improved page load times on btek by about 20-25 ms. :-)
Reopening. Unfortunately, this broke a common usage of javascript on the web:

  <script>
      document.write('<img src="url_for_some_image">');
  </script>

It also breaks the case where multiple images are written into the page. What 
happens is that the last image written is not show on screen. I'll attach a 
short testcase in a moment. I've verified that it is this checkin that is the 
cause by backing out and reapplying the checking in my tree (win2k; 03/21 6pm 
pull).

nsbeta1, mozilla1.0 for fixing the regression.

Hate to see the perf gain go away (although possibly it was an illusory gain,
since if content is now missing from certain pages, then it's not a valid test.
I happened to notice this while running the pageload test and saw a few pages 
that I knew were missing content, most notably the moviefone.com page).
Status: RESOLVED → REOPENED
Keywords: mozilla1.0, nsbeta1
Resolution: FIXED → ---
John, we already have bug 132673 for that problem. I was afraid the performance
win was bogus. I tried to verify that no content was missing (especially after
the previous patch did just that), but I didn't notice any. I hope we can fix
both problems (.innerHTML and document.write), so I'll close this one and use
bug 132673 for now. If there is no solution I'll back out and reopen this one.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
document.body.innerHTML works fine for me. However the testcase john provided
does not work. I will refer to bug 132673 for this problem. If everyone agrees i
will close this bug. 

Sorry for missing bug 132673 before I reopened this one. Yes, leave this bug 
to address the original innerHtml issue, and bug 132673 to address the 
fallout document.write issue. I wouldn't "close" this bug, as peterv noted, 
since if there isn't a fix for 132673, then a new fix will be needed for this
bug.
Reopening this, I'm backing out the patch. document.write is used way more often
and I don't want to leave it broken any longer.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 130122
Patch backed out, Tp times went up a little as expected. I probably won't be
able to fix this for 1.0.
Status: REOPENED → ASSIGNED
Comment on attachment 74520 [details] [diff] [review]
Fix v2

removing approval so this doesn't look like a patch waiting to land.
Attachment #74520 - Flags: approval+
nsbeta1-

Before you renominate, please query bugs marked nsbeta1+ keyword with [ADT# RTM]
in status whiteboard (where # is a number between 1 and 3) and make sure that
this bug is at least as important as those.
Keywords: nsbeta1 → nsbeta1-
Keywords: mozilla1.0, nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.1beta
Keywords: mozilla1.1
Target Milestone: mozilla1.1beta → mozilla1.3alpha
Attached file Minimized testcase — 

    
    

    
  
WFM 2002121608, Windows 2000.

    
    

    
  
wfm trunk build 2002121908 on winxp pro sp1
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → WORKSFORME

    
  
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: