Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[FIX]Strange rendering bug when javascript is executed: everything is rendered twice

RESOLVED FIXED

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Yongqian, Assigned: bz)

Tracking

({fixed1.9.1, regression, verified1.9.0.12})

unspecified
x86
Windows Vista
fixed1.9.1, regression, verified1.9.0.12
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.12 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments)

36.21 KB, application/zip
Details
7.03 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
sicking
: superreview+
Details | Diff | Splinter Review
7.44 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 373549 [details]
xpi + html to reproduce bug
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.
Component: General → Layout
Keywords: regression
Product: Firefox → Core
QA Contact: general → layout
CC Mr 175+ Items for both bugs
(Assignee)

Comment 5

8 years ago
Looks like a regression from bug 392318 somehow.
Blocks: 392318
(Assignee)

Comment 6

8 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

8 years ago
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...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #374344 - Flags: superreview?(jonas)
Attachment #374344 - Flags: review?(bent.mozilla)
(Assignee)

Comment 8

8 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

8 years ago
Flags: in-testsuite?
Attachment #374344 - Flags: review?(bent.mozilla) → review+
Comment on attachment 374344 [details] [diff] [review]
Proposed fix

makes sense to me, and looks good.
(Reporter)

Updated

8 years ago
Blocks: 490187
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

8 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

8 years ago
Is Bug 490187 exploitable? If so, maybe it should be marked as having security implications as well.
Flags: wanted1.9.0.x+
Whiteboard: [sg:critical?]
(Assignee)

Comment 13

8 years ago
I don't think that null deref is exploitable per se, no.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(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

8 years ago
Yeah, that's sorta my take on it too.  ;)
Jonas, can you review this?
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
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

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/7eac29fbea15
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

8 years ago
Created attachment 377946 [details] [diff] [review]
1.9.0 fix
Attachment #377946 - Flags: approval1.9.0.11?
(Assignee)

Comment 20

8 years ago
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c0b1bdf8e29
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs landing] → [sg:critical?]
Attachment #377946 - Flags: approval1.9.0.11?
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

8 years ago
Attachment #377946 - Flags: approval1.9.0.12?
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

8 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
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
Group: core-security
You need to log in before you can comment on or make changes to this bug.