Last Comment Bug 489050 - [FIX]Strange rendering bug when javascript is executed: everything is rendered twice
: [FIX]Strange rendering bug when javascript is executed: everything is rendere...
Status: RESOLVED FIXED
[sg:critical?]
: fixed1.9.1, regression, verified1.9.0.12
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: x86 Windows Vista
: P2 normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 396226
Blocks: 392318 490187
  Show dependency treegraph
 
Reported: 2009-04-19 00:08 PDT by Yongqian
Modified: 2009-07-21 17:13 PDT (History)
19 users (show)
dbaron: blocking1.9.1+
dveditz: blocking1.9.0.12+
dveditz: wanted1.9.0.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
xpi + html to reproduce bug (36.21 KB, application/zip)
2009-04-19 00:09 PDT, Yongqian
no flags Details
Proposed fix (7.03 KB, patch)
2009-04-23 14:00 PDT, Boris Zbarsky [:bz] (still a bit busy)
bent.mozilla: review+
jonas: superreview+
Details | Diff | Splinter Review
1.9.0 fix (7.44 KB, patch)
2009-05-17 10:29 PDT, Boris Zbarsky [:bz] (still a bit busy)
dveditz: approval1.9.0.12+
Details | Diff | Splinter Review

Description Yongqian 2009-04-19 00:08:30 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)

I am developing an extension which uses the jQuery library. The reverent aspect of the functionality can be thought of as being a simpler version of Greasemonkey. When a page loads, I execute jQuery in the context of the page and then execute a few other scripts that uses jQuery to modify the page DOM to provide the user with a few convenient features.

There is a bug however with Firefox such that executing jQuery causes certain pages to render twice, as if the entire page has been copy and pasted. I have provided a stripped down version of my extension which demonstrates this effect. In this version, jQuery is injected into the page in the form of a script tag but it also occurs if jQuery is executed by evalInSandbox.

I do not believe this is a jQuery bug since it works fine when I include it in the html file. It only triggers the bug when injected or eval'ed in evalInSandbox.

Reproducible: Always

Steps to Reproduce:
1. Install the provided xpi
2. open up the included html or go to http://www.bagelwood.com/forums/
3. You may need to refresh the page
4. Everything is rendered twice!
Actual Results:  
Everything is rendered twice!

Expected Results:  
Everything is rendered only once.
Comment 1 Yongqian 2009-04-19 00:09:36 PDT
Created attachment 373549 [details]
xpi + html to reproduce bug
Comment 2 Brian Carpenter [:geeknik] 2009-04-19 00:18:06 PDT
I installed the extension under Firefox 3.5b4pre Build 20090418203959 and went to the forums and I did not see this behavior exhibited.  Could you attach a screenshot for those of us that don't have 3.0.8 installed?
Comment 3 Ria Klaassen (not reading all bugmail) 2009-04-19 09:39:16 PDT
Yeah, I was able to reproduce it (on Windows). I see http://www.bagelwood.com/forums/ repeated on the same page. :)
It has also a regression range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1190341140&maxdate=1190342999
So it is caused by Bug 396099 or Bug 392318.
Comment 4 Matthias Versen [:Matti] 2009-04-19 11:27:59 PDT
CC Mr 175+ Items for both bugs
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2009-04-23 13:23:12 PDT
Looks like a regression from bug 392318 somehow.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2009-04-23 13:43:02 PDT
Here's the stack to us double-notifying:

#0  PresShell::ContentAppended (this=0x151ec00, aDocument=0x141e600, aContainer=0x17145620, aNewIndexInContainer=2) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:4839
#1  0x11b79e1e in nsNodeUtils::ContentAppended (aContainer=0x17145620, aNewIndexInContainer=2) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsNodeUtils.cpp:119
#2  0x11ae4eb4 in nsContentSink::NotifyAppend (this=0x14e6c00, aContainer=0x17145620, aStartIndex=2) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsContentSink.cpp:1352
#3  0x11c9e585 in SinkContext::FlushTags (this=0x1d7e35b0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/html/document/src/nsHTMLContentSink.cpp:1383
#4  0x11c9e67a in HTMLContentSink::FlushTags (this=0x14e6c00) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/html/document/src/nsHTMLContentSink.cpp:3219
#5  0x11c9e081 in HTMLContentSink::FlushPendingNotifications (this=0x14e6c00, aType=Flush_ContentAndNotify) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/html/document/src/nsHTMLContentSink.cpp:3198
#6  0x11b2dc54 in nsDocument::FlushPendingNotifications (this=0x141e600, aType=Flush_ContentAndNotify) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsDocument.cpp:6240
#7  0x11b19f83 in nsDocument::GetElementByIdInternal (this=0x141e600, aID=0x1d74e480) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsDocument.cpp:3761
#8  0x11b224e4 in nsDocument::GetElementById (this=0x141e600, aElementId=@0xbfffc118, aReturn=0xbfffc114) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsDocument.cpp:3785
#9  0x11cab629 in nsHTMLDocument::GetElementById (this=0x141e600, aElementId=@0xbfffc118, aReturn=0xbfffc114) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/html/document/src/nsHTMLDocument.cpp:2321

The issue is that the script runs when DOMContentLoaded fires; at this point we have mWeakSink but not mParser.  So when the script first modifies the DOM (by inserting stuff before the <body>) and then triggers a flush, we try to flush the sink and end up notifying on the <body> a second time.  we end up with two frames for the <body> and everything under it.

There's a nice XXX comment in HTMLContentSink::DidBuildModel:

  // XXXbz I wonder whether we could End() our contexts here too, or something,
  // just to make sure we no longer notify...
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2009-04-23 14:00:25 PDT
Created attachment 374344 [details] [diff] [review]
Proposed fix

I'll try to think of a way to test this; I'd need a stylesheet that's guaranteed to load later than DOMContentLoaded fires...
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2009-04-23 14:01:28 PDT
We should fix this on branches too.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-04-23 17:12:54 PDT
Comment on attachment 374344 [details] [diff] [review]
Proposed fix

makes sense to me, and looks good.
Comment 10 Daniel Veditz [:dveditz] 2009-05-01 10:43:23 PDT
bz: what's the security implication of rendering everything twice?

With 1.9.0.11 code-freeze immanent and this not landed on trunk we're going to push this to .12 unless you say it's super scary and obvious to attackers.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2009-05-01 11:00:33 PDT
The security implication is that you can end up with frames in the frame tree that are pointing to nodes that are no longer in the document.  Bug 490187 is a crash that's likely a result of something like that.

I _think_ on 1.9.0 those are mostly limited to null-deref issues, even for subdocument frames, but I haven't kept up on that.
Comment 12 Yongqian 2009-05-01 18:42:54 PDT
Is Bug 490187 exploitable? If so, maybe it should be marked as having security implications as well.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2009-05-03 11:26:54 PDT
I don't think that null deref is exploitable per se, no.
Comment 14 Daniel Veditz [:dveditz] 2009-05-08 10:45:19 PDT
(In reply to comment #11)
> I _think_ on 1.9.0 those are mostly limited to null-deref issues, even for
> subdocument frames, but I haven't kept up on that.

We should just take this fix in 1.9.0 then. "mostly" null-derefs "I think" is not entirely reassuring :-)
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2009-05-08 11:50:50 PDT
Yeah, that's sorta my take on it too.  ;)
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-05-11 17:39:21 PDT
Jonas, can you review this?
Comment 17 Damon Sicore (:damons) 2009-05-15 11:34:36 PDT
Walking over to Jonas' desk with a stick.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2009-05-17 10:15:51 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/7eac29fbea15
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2009-05-17 10:29:44 PDT
Created attachment 377946 [details] [diff] [review]
1.9.0 fix
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2009-05-17 12:47:23 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c0b1bdf8e29
Comment 21 Samuel Sidler (old account; do not CC) 2009-05-18 17:18:32 PDT
Comment on attachment 377946 [details] [diff] [review]
1.9.0 fix

Boris, we're code frozen for 1.9.0.11. Please re-nom for 1.9.0.12.
Comment 22 Daniel Veditz [:dveditz] 2009-05-22 11:00:05 PDT
Comment on attachment 377946 [details] [diff] [review]
1.9.0 fix

Approved for 1.9.0.12, a=dveditz for release-drivers
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2009-06-01 09:11:42 PDT
Checking in content/base/src/nsContentSink.h;
/cvsroot/mozilla/content/base/src/nsContentSink.h,v  <--  nsContentSink.h
new revision: 1.45; previous revision: 1.44
done
Checking in content/html/document/src/nsHTMLContentSink.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLContentSink.cpp,v  <--  nsHTMLContentSink.cpp
new revision: 3.811; previous revision: 3.810
done
Checking in content/xml/document/src/nsXMLContentSink.cpp;
/cvsroot/mozilla/content/xml/document/src/nsXMLContentSink.cpp,v  <--  nsXMLContentSink.cpp
new revision: 1.450; previous revision: 1.449
done
Comment 24 Al Billings [:abillings] 2009-06-30 12:57:03 PDT
Verified for 1.9.0.12 with testcase and xpi file. Fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729) and reproduces in 3.0.11.

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