Last Comment Bug 312695 - [FIX]DIV content is rendered twice
: [FIX]DIV content is rendered twice
Status: RESOLVED FIXED
: fixed1.8, regression, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.8rc1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 312847 314776 315189 315999
Blocks: 230170
  Show dependency treegraph
 
Reported: 2005-10-17 02:57 PDT by Miroslav Juhos
Modified: 2008-07-31 02:43 PDT (History)
7 users (show)
mscott: blocking1.8rc1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reporter's testcase (330 bytes, text/html)
2005-10-17 06:33 PDT, Uri Bernstein (Google)
no flags Details
Modified testcase that also shows the problem in builds from before restyle batching (291 bytes, text/html)
2005-10-17 13:13 PDT, Boris Zbarsky [:bz]
no flags Details
Proposed patch (4.53 KB, patch)
2005-10-17 15:13 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
asa: approval1.8rc1+
Details | Diff | Review
insertBefore test case (466 bytes, text/html)
2005-11-02 06:50 PST, Miroslav Juhos
no flags Details
replaceChild test case (466 bytes, text/html)
2005-11-02 06:51 PST, Miroslav Juhos
no flags Details
appendChild test case (408 bytes, text/html)
2005-11-02 06:52 PST, Miroslav Juhos
no flags Details

Description Miroslav Juhos 2005-10-17 02:57:58 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Display hidden DIV element and set STYLE to this element.

If you change order of this operation, it works fine.

Reproducible: Always

Steps to Reproduce:
Try this code:
<html>
   <head>
     <script>
     onload = function() {
      document.getElementById("message").style.display = "block";
      document.getElementById("message").innerHTML =
"content<style>#message{color:blue}</style>";
     }
     </script>
  </head>

<body>
   <div id="message" style="display:none;"></div>
</body>
</html>

Tested on WinXP and on MacOS X too.
Actual Results:  
Firefox displays text "contentcontent".

Expected Results:  
Firefox will display text "content".
Comment 1 Uri Bernstein (Google) 2005-10-17 06:33:45 PDT
Created attachment 199796 [details]
reporter's testcase
Comment 2 Uri Bernstein (Google) 2005-10-17 06:35:54 PDT
Happens on trunk as well. Does not happen in Firefox 1.0.
Comment 3 Uri Bernstein (Google) 2005-10-17 07:07:34 PDT
The regression range is between 2004-08-09-09 and 2004-08-10-15. Maybe bug 27382?

I'm confirming this in Core/DOM, which is my best guess.
Comment 4 Boris Zbarsky [:bz] 2005-10-17 07:41:56 PDT
Regression from bug 230170, looks like.  I thought I'd fixed this in bug 309986,
but clearly I didn't get all cases.  I'll debug tonight.
Comment 5 Asa Dotzler [:asa] 2005-10-17 11:41:40 PDT
Let us know what your debugging shows tonight (we might take a fix if it was
very low risk) and we're re-evaluate the status tomorrow. 
Comment 6 Boris Zbarsky [:bz] 2005-10-17 12:11:40 PDT
Sounds good.
Comment 7 Boris Zbarsky [:bz] 2005-10-17 13:13:01 PDT
Created attachment 199837 [details]
Modified testcase that also shows the problem in builds from before restyle batching
Comment 8 Boris Zbarsky [:bz] 2005-10-17 13:22:58 PDT
So this is a DOM bug after all.  The problem is that when setting innerHTML we
use a document fragment.  The sequence of events here is:

1)  Insert text node into div
2)  Insert style node into div.  This triggers a parse of the sheet and a style
reresolve.  Since the style of the div changes, we rebuild the frames for the
subtree rooted at it, and construct a frame for the text.
3)  DOM notifies that the text got appended.  We construct _another_ frame for
the text.

I see three possible solutions offhand:

1)  Just notify for every single node in the fragment when inserting a fragment.
 This is probably safest from a correctness point of view, but might have
performance issues, both for setting .innerHTML and for editor.
2)  Handle the addition of a new sheet to the document asynchronously (that is,
when StyleSheetAdded comes in, don't do a reresolve immediately, or even at
EndUpdate, but rather just stick a pending reresolve in the frame constructor).
 This is what I think I'd like to do on trunk, in general.  But I'd not want to
do this on branch.
3)  Hook up an document observer for the duration of inserting a fragment and do
a simpler version what the HTML content sink does (notify on everything we've
inserted so far when BeginUpdate happens, and then make sure to not notify on
those nodes again).

For branch I think I like #3, actually.  It should be safe enough that I'd be ok
with doing it on branch even at this point...
Comment 9 Boris Zbarsky [:bz] 2005-10-17 15:13:01 PDT
Created attachment 199855 [details] [diff] [review]
Proposed patch

This doesn't handle scripts in the fragment very well, just like the existing
code, but in the common cases (editor, innerHTML) they won't run in any case. 
This should handle anything that does non-content updates just dandy.  For 1.9
we really need to move toward not mutating things in any way from inside
BindToTree... or something.
Comment 10 Boris Zbarsky [:bz] 2005-10-17 15:13:53 PDT
I'll file followup bugs on the further work that needs doing here.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-17 16:23:34 PDT
Comment on attachment 199855 [details] [diff] [review]
Proposed patch

r+sr=jst
Comment 12 Boris Zbarsky [:bz] 2005-10-17 18:55:12 PDT
Fixed on trunk.
Comment 13 Boris Zbarsky [:bz] 2005-10-17 18:58:06 PDT
Comment on attachment 199855 [details] [diff] [review]
Proposed patch

Requesting branch approval.  This should be fairly safe -- all I'm doing in the
patch is notifying any time there's an update triggered by the content
addition, and notifying on what's left in the end, instead of notifying on
everything all together at the end.  This fixes various cases where innerHTML
adds stylesheets, basically.  There is some minor beneficial effect on actual
manipulations with document fragments (which no one does, really) that have
scripts in them -- for very simple scripts we might now behave better.	But the
main idea is fixing this innerHTML with stylesheet thing.
Comment 14 Boris Zbarsky [:bz] 2005-10-18 06:34:59 PDT
Fixed on 1.8 branch.
Comment 15 Scott MacGregor 2005-10-18 09:19:13 PDT
making the blocking flag reflect reality for this bug.
Comment 16 Miroslav Juhos 2005-11-02 06:48:15 PST
There is problem when is added HTML code to document by DOM methods:
 - appendChild
 - insertBefore
 - replaceChild
maybe by other methods.
See attached testcases.

Tested on Firefox 1.5.RC1
Mozilla/5.0 (Windows; U; Windows NT 5.1; cs; rv:1.8) Gecko/20051025 Firefox/1.5
Comment 17 Miroslav Juhos 2005-11-02 06:50:25 PST
Created attachment 201627 [details]
insertBefore test case
Comment 18 Miroslav Juhos 2005-11-02 06:51:20 PST
Created attachment 201628 [details]
replaceChild test case
Comment 19 Miroslav Juhos 2005-11-02 06:52:13 PST
Created attachment 201630 [details]
appendChild test case
Comment 20 Boris Zbarsky [:bz] 2005-11-02 07:50:56 PST
Please file a followup bug on that?  And cc me on it?  That's a subtly different problem, and one we're not going to be able to fix for 1.8 at this point...

This bug, as originally filed and with original testcase, is fixed.
Comment 21 Miroslav Juhos 2005-11-02 08:06:51 PST
Yes, I made bug #314776.
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-11 10:20:58 PST
The extra problems with button 5 went away on the trunk between 2005-11-06-04-trunk and 2005-11-07-04-trunk (Linux firefox).
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-11 10:21:22 PST
Er, sorry, wrong bug.

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