document.body.innerHTML is broken

RESOLVED WORKSFORME

Status

()

P2
normal
RESOLVED WORKSFORME
17 years ago
11 years ago

People

(Reporter: giscardg, Assigned: peterv)

Tracking

Trunk
mozilla1.3alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

17 years ago
<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

Updated

17 years ago
Priority: -- → P3
(Assignee)

Comment 2

17 years ago
Posted 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.
(Assignee)

Comment 3

17 years ago
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 5

17 years ago
Comment on attachment 73771 [details] [diff] [review]
Fix v1

r=fabian
Attachment #73771 - Flags: review+
(Assignee)

Comment 6

17 years ago
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+
(Assignee)

Comment 7

17 years ago
Posted patch Fix v2Splinter 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 9

17 years ago
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+
(Assignee)

Comment 11

17 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 13

17 years ago
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 → ---
(Assignee)

Comment 15

17 years ago
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
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 16

17 years ago
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. 

Comment 18

17 years ago
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.
(Assignee)

Comment 19

17 years ago
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 → ---

Updated

17 years ago
Blocks: 130122
(Assignee)

Comment 20

17 years ago
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-
(Assignee)

Updated

17 years ago
Keywords: mozilla1.0, nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.1beta

Updated

17 years ago
Keywords: mozilla1.1
(Assignee)

Updated

17 years ago
Target Milestone: mozilla1.1beta → mozilla1.3alpha

Comment 23

17 years ago
Posted file Minimized testcase

Comment 24

17 years ago
WFM 2002121608, Windows 2000.

Comment 25

17 years ago
wfm trunk build 2002121908 on winxp pro sp1
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → WORKSFORME

Updated

11 years ago
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.