Closed Bug 326644 Opened 14 years ago Closed 14 years ago

Crash when changing enumerated properties of objects in xul

Categories

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

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: mats)

Details

(4 keywords, Whiteboard: [sg:critical?][rft-dl])

Attachments

(6 files, 1 obsolete file)

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.
Attached file testcase
Attached file 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
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.  :(
Attached file 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.
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.
Attached file 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.
Attached file 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
Attached patch Like so? (obsolete) — Splinter Review
This fixes the crash for all the testcases and runs without assertions.
Keywords: crash
OS: Windows XP → All
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?  :(
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
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).
Attachment #211579 - Attachment is obsolete: true
Attached patch wallpaperSplinter Review
FWIW, this fixes the crash

Bug 280541, bug 251197 and bug 165110 appears related...
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?
Attachment #211699 - Flags: superreview+
Attachment #211699 - Flags: review+
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?
Attachment #211699 - Flags: review+
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.
Flags: blocking1.7.14?
Flags: blocking1.7.13?
Flags: blocking1.7.13-
Flags: blocking-aviary1.0.9?
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8-
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.
Attachment #211699 - Flags: approval-branch-1.8.1+
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Assignee: general → mats.palmgren
(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.
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
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.2-
Flags: blocking1.8.0.2+
For what it's worth, this crash is probably exploitable, so I'm not sure it matters whether it's a topcrash...
> 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
Group: security
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2-
Whiteboard: [sg:??]
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 211699 [details] [diff] [review]
wallpaper

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #211699 - Flags: approval1.8.0.2+
Can we get this landed soon please?
By soon I mean tonight, tomorrow the 1.8.0.2 approval will be removed.
Checked in on trunk and the two 1.8 branches.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [sg:??] → [sg:??][rft-dl]
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
meant to reopen and removed fixed1.8.02 keyword
Status: RESOLVED → REOPENED
Keywords: fixed1.8.0.2
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED
okay then, verified 1.8.0.2 as the minimised testcase doesn't crash
Whiteboard: [sg:??][rft-dl] → [sg:critical?][rft-dl]
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
forgot to mention, no crash with https://bugzilla.mozilla.org/attachment.cgi?id=211573 either.
Group: security
crash test landed
http://hg.mozilla.org/mozilla-central/rev/fea7278a05db
Flags: in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.