375399 - Crash [@ nsElementSH::PostCreate] when removing window when accessing xul:tree in xul:tabs onselect in svg:foreignObject

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

Attachment: iframe needed for testcase

See upcoming testcase, which crashes current trunk and branch builds.

Talkback ID: TB30587921Q
0x02a8e765
nsElementSH::PostCreate  [mozilla/dom/src/base/nsdomclassinfo.cpp, line 6836]
XPCWrappedNative::GetNewOrUsed  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 541]
XPCConvert::NativeInterface2JSObject  [mozilla/js/src/xpconnect/src/xpcconvert.cpp, line 1104]
nsXPConnect::WrapNative  [mozilla/js/src/xpconnect/src/nsxpconnect.cpp, line 996]
nsDOMClassInfo::WrapNative  [mozilla/dom/src/base/nsdomclassinfo.cpp, line 1484]
nsNodeSH::PreCreate  [mozilla/dom/src/base/nsdomclassinfo.cpp, line 6388]
XPCWrappedNative::GetNewOrUsed  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 398]
XPCConvert::NativeInterface2JSObject  [mozilla/js/src/xpconnect/src/xpcconvert.cpp, line 1104]
XPCConvert::NativeData2JS  [mozilla/js/src/xpconnect/src/xpcconvert.cpp, line 480]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2337]

Talkback ID for branch: TB30587979K

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Note, only non-debug-builds crash.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

hmm, this wasn't the kind of crash I thought.
I'll look at this anyway, but if someone notices immediately what is
going wrong, feel free to take this.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So mRuleNode is deleted before nsStyleContext?

Comment 5

[Samuel Sidler (old account; do not CC)]

Attachment: Branch log from 2.0.0.4 RC1

Comment 6

[Samuel Sidler (old account; do not CC)]

Attachment: Mac Trunk log from 2007-04-28

Based on the Mac branch crash log above, I don't think this is exploitable on the branch, but based on this trunk log, it could be exploitable there.

I'm also crashing in a completely different place than Martijn. Not sure if that's just Mac vs Windows. (And the branch and trunk crashes happen in different places as well but that's partially expected.)

Comment 7

[Samuel Sidler (old account; do not CC)]

Boris, can you take a look at this? Or who's the best person to look at it?

Comment 8

[Samuel Sidler (old account; do not CC)]

Attachment: Mac Branch Debug log from 1.8 branch (~2007050317)

This log from a debug build suggests this might be exploitable on the 1.8 branch, so I retract my above statement.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I won't be able to look at this on a mac for a few weeks, probably...  Not sure who's best to look, to be honest.  :(

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

This is not a mac-only crash. (but non-debug-only maybe)
I'm really not familiar with stylecontext's memory handling, who owns what and how long. What should make sure that mRuleNode isn't deleted before stylecontext? dbaron?

#0  0x00000000006d0cf0 in ?? ()
#1  0x00002aaab0c182d6 in nsStyleContext::Destroy () from /home/smaug/mozilla/mozilla_cvs/mozilla/ff_build_non_debug/dist/bin/components/libgklayout.so
#2  0x00002aaab0e0a4e8 in nsElementSH::PostCreate () from /home/smaug/mozilla/mozilla_cvs/mozilla/ff_build_non_debug/dist/bin/components/libgklayout.so
#3  0x00002aaaaf519a30 in XPCWrappedNative::GetNewOrUsed ()
   from /home/smaug/mozilla/mozilla_cvs/mozilla/ff_build_non_debug/dist/bin/components/libxpconnect.so

Comment 11

[Samuel Sidler (old account; do not CC)]

My crash log from comment 8 (attachment 263681 [details]) is from a branch debug build, so I don't think this is non-debug-only.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I'm really not familiar with stylecontext's memory handling, who owns what
> and how long.

Style contexts are refcounted, but the memory is allocated from the presshell arena.  Which means if a style context outlives its presshell, it's toast.

Rule nodes are garbage collected, basically.  Every so often we nuke all the ones that are not pointed to by any style contexts.  See the NotifyStyleContextDestroyed() call in nsStyleContext::~nsStyleContext.  Again, they shouldn't be outliving the presshell.

Which brings us back to this bug.  Patch coming up, I think.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Does this fix the crash?

At a guess, the SVG node makes us not create frames for the <tabs>, so the binding is attached when we get the node, and the constructor fires the event, which kills off the presshell, etc.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 263698 [details] [diff] [review]
Does this fix the crash?

That fixes it. and the patch makes sense to me :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w for ease of review

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 263698 [details] [diff] [review]
Does this fix the crash?

It's worth doing this on branches.  It's a very safe fix.

Comment 18

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070505 Minefield/3.0a5pre

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 263698 [details] [diff] [review]
Does this fix the crash?

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 21

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.5 using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.5pre) Gecko/2007070403 BonEcho/2.0.0.5pre on Fedora F7 and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/20070704 BonEcho/2.0.0.5pre ID:2007070403 - no crash with the testcase from this bug - adding verified keyword

Comment 22

[juan becerra [:juanb]]

I wasn't able to reproduce the crash on Thunderbird 15012 or 15013 using the testcase #1. 

Comment 23

[Carsten Book [:Tomcat]]

no crash on Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.13pre) Gecko/20070822 Firefox/1.5.0.13pre with the testcase provided in this bug. Adding verified keyword for 1.8.0.13

Comment 24

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/3a53dc42292c