326644 - Crash when changing enumerated properties of objects in xul

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes after waiting for a few seconds.
Unforunately, I haven't been able to minimise the testcase yet.
It also crashes Mozilla1.7.12.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: backtrace

Backtrace (for a bit of an old debug build):

Program received signal SIGSEGV, Segmentation fault.
0x051f332b in nsAttributeChildList::GetLength(unsigned*) (this=0x104a1858,
    aLength=0x22ea68)
---Type <return> to continue, or q <return> to quit---
    at c:/mozilla/mozilla/content/base/src/nsDOMAttribute.cpp:808
warning: Source file is more recent than executable.

808
Current language:  auto; currently c++
(gdb) bt
#0  0x051f332b in nsAttributeChildList::GetLength(unsigned*) (
    this=0x104a1858, aLength=0x22ea68)
    at c:/mozilla/mozilla/content/base/src/nsDOMAttribute.cpp:808
#1  0x6ff596b1 in _XPTC_InvokeByIndex ()
    at ../../../../../../dist/include/xpcom/xptcall.h:123
#2  0x6ff59686 in XPTC_InvokeByIndex (that=0x104a1858, methodIndex=4,
    paramCount=1, params=0x22ea68)
    at c:/mozilla/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_gcc_x86_u
nix.cpp:125
#3  0x6703f85d in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative
::CallMode) (ccx=@0x22eb78, mode=CALL_GETTER)
    at c:/mozilla/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2151
#4  0x67062771 in XPCWrappedNative::GetAttribute(XPCCallContext&) (
    ccx=@0x22eb78)
    at c:/mozilla/mozilla/js/src/xpconnect/src/xpcprivate.h:1980
etc

Comment 3

[Boris Zbarsky [:bzbarsky]]

With a current debug build I see all sorts of stuff broken in frame construction land, and I die on a dead frame that got readded to the primary frame map.

Anything that would minimize this testcase would be wonderful.  :(

Comment 4

[Aiko]

Attachment: testcase 2

Using this testcase I've found the following: Firefox 1.5.0.1 crashes if I test "prefix". The latest trunk build (20060210) crashes if I use "hidden" instead.

Comment 5

[Martijn Wargers (dead)]

Thanks, Seno!
Ok, when I stick an alert(aObj.nodeName); in testcase2 when "if (propName == 'hidden')", then I get appr. 6 times 'window', then I get 'popupgroup' and then 'tooltip' and then I crash.

When looking for 'popupgroup' ( http://lxr.mozilla.org/seamonkey/search?string=popupgroup ):
I get this:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsDocElementBoxFrame.cpp#116
So it seems that the script somehow manages to access native anonymous content here, which doesn't seem like a good idea.
The actual crash, though, might be related to bug 326529, I guess.

Comment 6

[Martijn Wargers (dead)]

Attachment: Testcase3, minimised

Ok, so I can access the anonymous popupgroup by calling document.documentElement.boxObject.firstChild.

Doing this:
document.documentElement.boxObject.firstChild.hidden = true;
document.documentElement.boxObject.firstChild.tooltip = 'test';
is what makes Mozilla crash.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Frame dump

The restyle leads to a reframing of the 'popupgroup' which causes a
call to DeletingFrameSubtree to clear out frame references etc..
This is ok, the problem is that nsPopupSetFrame has a private
list of frames that is unknown to the frame constructor:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsPopupSetFrame.h&rev=1.45&root=/cvsroot&mark=59-61,143#59
This causes the assertions.
(The #PopupFrameList<...> is a dump of this list)

The crash is because tooltip placeholder is not removed - its out-of-flow
is destroyed in nsPopupSetFrame::Destroy:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp&rev=1.131&root=/cvsroot&mark=181-184#164

Comment 8

[Mats Palmgren (inactive)]

Attachment: Like so?

This fixes the crash for all the testcases and runs without assertions.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Can't there be more than one popupgroup per document?  As in, how do you know the tooltip lives in your popupgroup?

Also, it would seem to me that the real problem is the access to anonymous content.  That's a really really bad idea -- we should not be allowing it.

Can someone remind me again WHY boxObjects are exposed to non-chrome script, given all the dangerous stuff that lives on them?  :(

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 211579 [details] [diff] [review]
Like so?

(In reply to comment #9)
> Can't there be more than one popupgroup per document?

Don't know. There can be more than one tooltip though, so this
wouldn't work anyway (e.g. bug 326529).

Comment 11

[Mats Palmgren (inactive)]

Attachment: wallpaper

FWIW, this fixes the crash

Bug 280541, bug 251197 and bug 165110 appears related...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211699 [details] [diff] [review]
wallpaper

I assume this makes us throw on this testcase at some point, right?

If so, excellent. Let's get this in (trunk and branches) and really work on getting bug 165110 for 1.9?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211699 [details] [diff] [review]
wallpaper

I assume this makes us throw on this testcase at some point, right?

If so, excellent. Let's get this in (trunk and branches) and really work on getting bug 165110 for 1.9?

Comment 14

[Tim Riley [:timr]]

Is this patch good for all branches (1.0.x, 1.7.x, 1.8.0.x)?  Let's land it on the trunk and then get it in for 1.0.9 and 1.7.14., 1.5.0.2, etc.  I am minusing this for 1.0.8/1.7.13 and nominating it for  1.0.9/1.7.14 since we are past code freeze.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211699 [details] [diff] [review]
wallpaper

> Is this patch good for all branches (1.0.x, 1.7.x, 1.8.0.x)? 

It should be; I'd have to actually try applying to those trees to know for sure.  If it applies, it's good.

Forgot to approve this for 1.8.1, btw.

Comment 16

[[e-mail]]

(In reply to comment #14)
>Is this patch good for all branches (1.0.x, 1.7.x, 1.8.0.x)?
I don't think it will apply to pre-1.8 branches, as I seem to recall that nsXULElement didn't use to inherit from nsGenericElement back then.

Comment 17

[Dave Liebreich [:davel]]

Please ask for 1803 approval for the patch if it should go on the 180 branch.  Since this is not a topcrash and we did not get it landed on the 180 branch prior to the code freeze, we're deferring to 1803

Comment 18

[Boris Zbarsky [:bzbarsky]]

For what it's worth, this crash is probably exploitable, so I'm not sure it matters whether it's a topcrash...

Comment 19

[Daniel Veditz [:dveditz]]

> probably exploitable, so I'm not sure it matters whether it's a topcrash...

If it's exploitable topcrash doesn't matter, but there weren't enough cues to indicate that in this bug--at least not at the speed which we have to go through these things given the size of the bug lists. You asked for blocking everywhere, but when you asked for approval it was pretty specific which made it look like maybe you'd changed your mind about the older branches.

renominating

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 211699 [details] [diff] [review]
wallpaper

approved for 1.8.0 branch, a=dveditz for drivers

Comment 21

[Daniel Veditz [:dveditz]]

Can we get this landed soon please?

Comment 22

[Daniel Veditz [:dveditz]]

By soon I mean tonight, tomorrow the 1.8.0.2 approval will be removed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk and the two 1.8 branches.

Comment 24

[Tracy Walker [:tracy]]

with Firefox 1.5.0.2 builds from 20060306
Windows and Mac:
testcase: crash TB ID's - 16013035, 16013058
testcase2: no crash
testcase3, minimised: no crash

Linux:
testcase: no crash
testcase2: no crash
testcase3, minimised: no crash

Comment 25

[Tracy Walker [:tracy]]

meant to reopen and removed fixed1.8.02 keyword

Comment 26

[Boris Zbarsky [:bzbarsky]]

Re-resolving.  The crash we were seeing to start with is fixed; the talkbacks point to a different crash.  We should get a new bug filed on that.  Note that the original testcase for this bug will just run until it crashes _somehow_; the minimal testcases are what's relevant.

Comment 27

[Tracy Walker [:tracy]]

okay then, verified 1.8.0.2 as the minimised testcase doesn't crash

Comment 28

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=211372
ff2b2 winxp, linux: no crash

https://bugzilla.mozilla.org/attachment.cgi?id=211482
ff2b2 winxp, linux: no crash

verified fixed 1.8

Comment 29

[Bob Clary [:bc] (inactive)]

forgot to mention, no crash with https://bugzilla.mozilla.org/attachment.cgi?id=211573 either.

Comment 30

[Bob Clary [:bc] (inactive)]

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