332807 - document.open() in a xbl constructor

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[georgi - hopefully not receiving bugspam]

document.open() in a xbl constructor causes crash, probably due to deleted
objects. debug builds try to call 0xdddddddd, optimized builds try to call
0x0 (on linux).

if document.close() is added in the constructor, the crash is different.

affects ff 1.5/trunk and at least seamonkey trunk from today.

testcase soon.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: crash

crash

Comment 2

[Martijn Wargers (dead)]

Testcase doesn't crash here, using latest windows trunk build.

Comment 3

[georgi - hopefully not receiving bugspam]

Attachment: windoze testcase

windoze testcase

Comment 4

[georgi - hopefully not receiving bugspam]

"windoze testcase" crashes seamonkey trunk under wine.
"crash" doesn't indeed.

Comment 5

[georgi - hopefully not receiving bugspam]

my stack:

[Switching to Thread 46912555133632 (LWP 21843)]
0x00002aaab25cc359 in nsCSSFrameConstructor::WipeContainingBlock (
    this=0x1206af0, aState=@0x7fffffbe0c50, aContainingBlock=0x1237ca8, 
    aFrame=0x1237ca8, aFrameList=0x12468d0)
    at /opt/joro/firefox-cvs/mozilla/layout/base/nsCSSFrameConstructor.cpp:13206
13206     nsIAtom* frameType = aFrame->GetType();
(gdb) p *aFrame
$1 = {<nsISupports> = {_vptr.nsISupports = 0x0}, mRect = {x = -572662307, 
    y = -572662307, width = -572662307, height = -572662307}, 
  mContent = 0xdddddddddddddddd, mStyleContext = 0xdddddddddddddddd, 
  mParent = 0xdddddddddddddddd, mNextSibling = 0xdddddddddddddddd, 
  mState = 3722304989}
(gdb) info stack
#0  0x00002aaab25cc359 in nsCSSFrameConstructor::WipeContainingBlock (
    this=0x1206af0, aState=@0x7fffffbe0c50, aContainingBlock=0x1237ca8, 
    aFrame=0x1237ca8, aFrameList=0x12468d0)
#1  0x00002aaab25ca913 in nsCSSFrameConstructor::ContentInserted (
    this=0x1206af0, aContainer=0x124b230, aChild=0x124b3d0, 
    aIndexInContainer=1, aFrameState=0x0, aInReinsertContent=0)
    at /opt/joro/firefox-cvs/mozilla/layout/base/nsCSSFrameConstructor.cpp:9640
#2  0x00002aaab2627b81 in PresShell::ContentInserted (this=0x12063a0, 
    aDocument=0x6a4ea0, aContainer=0x124b230, aChild=0x124b3d0, 
    aIndexInContainer=1)
    at /opt/joro/firefox-cvs/mozilla/layout/base/nsPresShell.cpp:5215
#3  0x00002aaab2a55b79 in nsXBLBindingRequest::DocumentLoaded (
    this=0x94bb78, aBindingDoc=0x1238d90)
    at /opt/joro/firefox-cvs/mozilla/content/xbl/src/nsXBLService.cpp:166
#4  0x00002aaab2a52a25 in nsXBLStreamListener::Load (this=0x123abd0, 
    aEvent=0x1223570)
    at /opt/joro/firefox-cvs/mozilla/content/xbl/src/nsXBLService.cpp:418
#5  0x00002aaab28e7905 in DispatchToInterface (aEvent=0x1223570, 
    aListener=0x123abd8, aMethod={__pfn = 0x21, __delta = 0}, 
    aIID=@0x2aaab2d3a510, aHasInterface=0x7fffffbe11dc)

Comment 6

[georgi - hopefully not receiving bugspam]

iirc someone suggested ditching xbl constructors from luserland.

if there is a bug "ditch the whole xbl for luserland" i would vote for it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

First issue is that any of this works in any way in XHTML.  I filed bug 332848 on that.

I assume that you can reproduce the issue as text/html too (possibly with the binding in a separate file)?  If so, bug 267833 might help.

Also, do you get similar crashes if you do the whole thing in an iframe and have the constructor remove the iframe from its parent document?  Or does it really have to be document.open that blows the document away?

Comment 8

[georgi - hopefully not receiving bugspam]

(In reply to comment #7)
> 
> I assume that you can reproduce the issue as text/html too (possibly with the
> binding in a separate file)?  If so, bug 267833 might help.
> 

it works the binding in a separate file.

> Also, do you get similar crashes if you do the whole thing in an iframe and
> have the constructor remove the iframe from its parent document?  Or does it
> really have to be document.open that blows the document away?
> 

have not tried this yet.
not sure if only document.open blows the document.

Comment 9

[georgi - hopefully not receiving bugspam]

(In reply to comment #7)

> I assume that you can reproduce the issue as text/html

it works as html.

Comment 10

[georgi - hopefully not receiving bugspam]

Attachment: xbl2-no-need-for-document.write

Comment 11

[georgi - hopefully not receiving bugspam]

Attachment: html2-crash

Comment 12

[georgi - hopefully not receiving bugspam]

(In reply to comment #7)
> 
> Also, do you get similar crashes if you do the whole thing in an iframe and
> have the constructor remove the iframe from its parent document?  Or does it
> really have to be document.open that blows the document away?
> 

another crash in the same place is via:

 <xul:iframe src='javascript:alert("doco "+parent.document.open())' width="100" height="100" />

this in xbl

Comment 13

[georgi - hopefully not receiving bugspam]

an html only (no xbl) iframe removing itself is Bug 332971

Comment 14

[georgi - hopefully not receiving bugspam]

Attachment: xbl removing iframe from ctor/dtor (xbl)

Comment 15

[georgi - hopefully not receiving bugspam]

Attachment: xbl removing iframe from ctor/dtor (inner html)

Comment 16

[georgi - hopefully not receiving bugspam]

Attachment: xbl removing iframe from ctor/dtor (open this)

Comment 17

[georgi - hopefully not receiving bugspam]

(In reply to comment #7)
> Also, do you get similar crashes if you do the whole thing in an iframe and
> have the constructor remove the iframe from its parent document?  

removing an iframe from ctor also crashes - check 
https://bugzilla.mozilla.org/attachment.cgi?id=217412
xbl removing iframe from ctor/dtor (open this)

>Or does it
> really have to be document.open that blows the document away?
> 

Comment 18

[Boris Zbarsky [:bzbarsky]]

> another crash in the same place is via:

Is that covered by one of the testcases in this bug?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, nevermind.  That's bug 332971.  OK.

Comment 20

[georgi - hopefully not receiving bugspam]

(In reply to comment #19)
> Ah, nevermind.  That's bug 332971.  OK.
> 

i am not quite sure the <xul:iframe> mess is the same as the above bug - in html sometimes reloads are needed, while this crashes one shot. may be wrong though.

testcase in this bug to come

Comment 21

[georgi - hopefully not receiving bugspam]

Attachment: xul iframe doing parent.open() - xbl

Comment 22

[georgi - hopefully not receiving bugspam]

Attachment: xul iframe doing parent.document.open() - open this

Comment 23

[georgi - hopefully not receiving bugspam]

btw, recent trunk is somewhat broken - window creation is *visibly* slow - and new windows stay white for 1-3 seconds.
on both the fox and the monkey.

is the problem in my box?

Comment 24

[georgi - hopefully not receiving bugspam]

(In reply to comment #20)
> i am not quite sure the <xul:iframe> mess is the same as the above bug - in
> html sometimes reloads are needed, while this crashes one shot. may be wrong
> though.
> 

though Bug 332971 is fixed, this abuse continues to crash.

bz: there are several testcases in this bug - should i fork the bug in several "issues" or "anomalies" as some call them?

Comment 25

[Boris Zbarsky [:bzbarsky]]

No, they all look like the same issue (bug 267833) to me.

Comment 26

[georgi - hopefully not receiving bugspam]

a small modification of html2-crash crashes with clear signs of using free()ed memory on firefox 2.0-latest

Comment 27

[georgi - hopefully not receiving bugspam]

mean that 'html2-crash' also crashes, but in not very scary way.

Comment 28

[Daniel Veditz [:dveditz]]

Critical security bugs must have owners. If you can't work on this bug please help us find another active owner for it.

Comment 29

[chris hofmann]

jonas/boris, any ideas on a fix?

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

same thing here, lets fix bug 267833 and see if this is still an issue after that.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Though for branches we might want something too... Not sure.  There are so many other ways branches can screw with XBL constructors...

Comment 32

[georgi - hopefully not receiving bugspam]

(In reply to comment #31)
> There are so many
> other ways branches can screw with XBL constructors...
> 

bz, do you suspect other code may screw by blowing objects? :)

Comment 33

[Boris Zbarsky [:bzbarsky]]

I'm not sure what you're asking.

Comment 34

[georgi - hopefully not receiving bugspam]

since you know other ways for abuse of xbl constructors do you know other methods/objects that may be abused in a similar manner?

Comment 35

[Boris Zbarsky [:bzbarsky]]

Not offhand, no.

Comment 36

[georgi - hopefully not receiving bugspam]

some xbl related stuff still crashes after bz's fix, investigating testcases.

in some cases (manually typing url in location) xbl destructors doesn't fire - this may be a sign of leak.

Comment 37

[georgi - hopefully not receiving bugspam]

https://bugzilla.mozilla.org/attachment.cgi?id=217441
xul iframe doing parent.document.open() - open this   (340 bytes, text/html)

still crashes

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: This seems to fix it...

We really need roc's compositor.  All this flushing is getting annoying.  ;)

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 258368 [details] [diff] [review]
This seems to fix it...

>+    // gotten destroyed.

I presume you mean |Destroy|ed and not destroyed.

If not, then somebody else should be holding an reference to the pres shell while calling a method on it.

r+sr=dbaron

Comment 40

[Boris Zbarsky [:bzbarsky]]

Yeah, I meant Destroy()ed.  I'll update the comment accordingly.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Checked in, with the improved comment.  We should land the tests once this becomes public, I guess...

Comment 42

[chris hofmann]

is this good stuff for the branch also?  any problems showing up in trunk baking?

Comment 43

[Boris Zbarsky [:bzbarsky]]

I'm not sure trunk is getting enough "baking" to be relevant, to be honest.  :(

As for branch... we could make this change, probably.  That still leaves bug   	267833 on branches, so I'm not sure it's worth it.

Comment 44

[Daniel Veditz [:dveditz]]

bug 267833 has landed on the 1.8 branch, but some of the testcases in this bug remain a problem (hangs rather than crashes). The patch here doesn't really apply so we'll need something else.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Daniel, which testcases hang for you?  I get a crash from "xul iframe doing parent.document.open() - open this" and nothing from the others...

It's pretty simple to port this patch to branch, for what it's worth.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch merge of the patch

I'm not sure I'll be able to drive this into the branch, esp. as it doesn't fix the parent.document.open() crash...

Comment 47

[Daniel Veditz [:dveditz]]

The crash in attachment 217441 [details] looks like a safe null-deref on the branch now. Maybe the fixes in but 267833 were sufficient to address the security problems.

Comment 48

[Daniel Veditz [:dveditz]]

What's the status of this bug on the branch? Do we need attachment 283134 [details] [diff] [review], can we live without it?

Comment 49

[Boris Zbarsky [:bzbarsky]]

The status is that the attachment is probably required to fix the remaining crashes, but not sufficient.

Comment 50

[Daniel Veditz [:dveditz]]

We should take the patch we can even if it's not a complete fix. Seems to make things better.

Comment 51

[Daniel Veditz [:dveditz]]

Guess this is assigned wrong since bz wrote the patches... 

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283134 [details] [diff] [review]
Branch merge of the patch

I guess let's get this in on branch... And I suppose file a followup bug on the remaining branch crashes, since QA won't be able to verify this one...

Comment 53

[Daniel Veditz [:dveditz]]

Comment on attachment 283134 [details] [diff] [review]
Branch merge of the patch

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 54

[Boris Zbarsky [:bzbarsky]]

Checked in on branch.

Comment 55

[Al Billings [:abillings - ex-MoCo]]

I'll check this in the release candidate for 2.0.0.13 tomorrow (when it is built) but I checked this in the 3/8 build from ftp://ftp.mozilla.org/pub/firefox/nightly/2008-03-08-03-mozilla1.8 and https://bugzilla.mozilla.org/attachment.cgi?id=217441 still crashes. That build should have been made after your check-in.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Al, ee comment 46 and comment 52.  This patch was necessary to fix the crashes on branch, but not sufficient; someone needs to file a separate branch-only bug on the remaining crashes.

Comment 57

[georgi - hopefully not receiving bugspam]

filed Bug 422338 on the crash and on clickable xul button in print preview

Comment 58

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #56)
> Al, ee comment 46 and comment 52.  This patch was necessary to fix the crashes
> on branch, but not sufficient; someone needs to file a separate branch-only bug
> on the remaining crashes.
> 

I see that was done. To be 100% clear, which crashes should this bug actually fix out of the attached repro cases?

Comment 59

[Boris Zbarsky [:bzbarsky]]

I have no idea, to be honest.  It might be none.  It'll fix some crash locations, but there might be others in every testcase.

Comment 60

[georgi - hopefully not receiving bugspam]

this bug is a clear example why i suggested killing xbl for untrusted content

Comment 61

[Alexander Sack]

Comment on attachment 283134 [details] [diff] [review]
Branch merge of the patch

distro patch. caillon please sign off.