36.21 KB, application/zip
7.03 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!): review+
|Details | Diff | Splinter Review|
7.44 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:220.127.116.11) 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:18.104.22.168) 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.
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?
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.
CC Mr 175+ Items for both bugs
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...
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...
We should fix this on branches too.
Comment on attachment 374344 [details] [diff] [review] Proposed fix makes sense to me, and looks good.
bz: what's the security implication of rendering everything twice? With 22.214.171.124 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.
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.
Is Bug 490187 exploitable? If so, maybe it should be marked as having security implications as well.
I don't think that null deref is exploitable per se, no.
(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 :-)
Yeah, that's sorta my take on it too. ;)
Jonas, can you review this?
Walking over to Jonas' desk with a stick.
Created attachment 377946 [details] [diff] [review] 1.9.0 fix
Comment on attachment 377946 [details] [diff] [review] 1.9.0 fix Boris, we're code frozen for 126.96.36.199. Please re-nom for 188.8.131.52.
Comment on attachment 377946 [details] [diff] [review] 1.9.0 fix Approved for 184.108.40.206, a=dveditz for release-drivers
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
Verified for 220.127.116.11 with testcase and xpi file. Fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729) and reproduces in 3.0.11.