Closed
Bug 489050
Opened 16 years ago
Closed 16 years ago
[FIX]Strange rendering bug when javascript is executed: everything is rendered twice
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
People
(Reporter: yongqli, Assigned: bzbarsky)
References
Details
(Keywords: fixed1.9.1, regression, verified1.9.0.12, Whiteboard: [sg:critical?])
Attachments
(3 files)
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
bent.mozilla
:
review+
sicking
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dveditz
:
approval1.9.0.12+
|
Details | Diff | Splinter Review |
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 2•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
CC Mr 175+ Items for both bugs
Assignee | ||
Comment 6•16 years ago
|
||
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...
Group: core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 7•16 years ago
|
||
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...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #374344 -
Flags: superreview?(jonas)
Attachment #374344 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 8•16 years ago
|
||
We should fix this on branches too.
Depends on: 396226
Flags: blocking1.9.1?
Flags: blocking1.9.0.11?
Summary: Strange rendering bug when javascript is executed: everything is rendered twice → [FIX]Strange rendering bug when javascript is executed: everything is rendered twice
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite?
Updated•16 years ago
|
Attachment #374344 -
Flags: review?(bent.mozilla) → review+
Comment on attachment 374344 [details] [diff] [review]
Proposed fix
makes sense to me, and looks good.
Comment 10•16 years ago
|
||
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.
Flags: blocking1.9.0.11? → blocking1.9.0.12?
Assignee | ||
Comment 11•16 years ago
|
||
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.
Reporter | ||
Comment 12•16 years ago
|
||
Is Bug 490187 exploitable? If so, maybe it should be marked as having security implications as well.
Updated•16 years ago
|
Flags: wanted1.9.0.x+
Whiteboard: [sg:critical?]
Assignee | ||
Comment 13•16 years ago
|
||
I don't think that null deref is exploitable per se, no.
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment 14•16 years ago
|
||
(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 :-)
Flags: blocking1.9.0.12? → blocking1.9.0.12+
Assignee | ||
Comment 15•16 years ago
|
||
Yeah, that's sorta my take on it too. ;)
Jonas, can you review this?
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
Comment 17•16 years ago
|
||
Walking over to Jonas' desk with a stick.
Attachment #374344 -
Flags: superreview?(jonas) → superreview+
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
Assignee | ||
Comment 18•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•16 years ago
|
||
Attachment #377946 -
Flags: approval1.9.0.11?
Assignee | ||
Comment 20•16 years ago
|
||
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs landing] → [sg:critical?]
Updated•16 years ago
|
Attachment #377946 -
Flags: approval1.9.0.11?
Comment 21•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #377946 -
Flags: approval1.9.0.12?
Comment 22•16 years ago
|
||
Comment on attachment 377946 [details] [diff] [review]
1.9.0 fix
Approved for 1.9.0.12, a=dveditz for release-drivers
Attachment #377946 -
Flags: approval1.9.0.12? → approval1.9.0.12+
Assignee | ||
Comment 23•16 years ago
|
||
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
Keywords: fixed1.9.0.12
Comment 24•16 years ago
|
||
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.
Keywords: fixed1.9.0.12 → verified1.9.0.12
Updated•15 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•