240542 - Firefox crashes on simple example

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm totally new in RDF so I won't be much helpful in making simple testcase.
I downloaded rdfds from xulplanet and runned example from taht site. It work's
perfect.
But to cover my needs I had to be able to add node to subnode, so I slightly
changed example.
1) I made Seq from catfish node
2) And in AddShark changed |fish| to point on catfish.

After this change I runned example and pressed "Add Shark" button and then
"SetTemplate" - quite good for now. But try pressing first "Set Template" and
after this "Add Shark" - everytime crash.

TalkBack id - TB17893Z

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can you attach your testcase to the bug or point to a URL that allows to
reproduce the bug?

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: modified example

sure
Steps to reproduce
1) unpack tar file to some folder under chrome
2) run firefox and launch xul file from chrome
3) press "Set Template"
4) press "Add Shark"
5) kaboom

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: simplified testcase

Ok. I finally simplified this testcase to reproduce crash without rdfds.js
library . Now it's one simple xul file with 3 JS functions. Hope that helps

Comment 4

[[e-mail]]

Basically NotifyListBoxBody stinks. It doesn't actually check that the parent of
a listitem is a listbox. This causes grief in two possible ways; the one here
QI's the parent's box object without null-checking the result; then when you
remove the listitem later it static casts the parent's frame.

Reporter, you shouldn't be using a listbox to display nested content.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So is there a static (or simply dynamic without the RDF garbage) way to
reproduce this crash?

Comment 6

[[e-mail]]

Well I couldn't get a dynamic test case to crash adding the list item, but
removing it crashed instantly. Note that removing sometimes makes it hang rather
than crash; the null comptr is a more obvious crash.

Comment 7

[[e-mail]]

Actally, removing it did make it crash eventually, it needs patience :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Neil, pointers to the code in question?  Stacks for the crashes?

Comment 9

[[e-mail]]

The advertised crash happens after
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#8625
fails because the box object isn't a list box object (in that particular case
the listitem parent is another listitem) so the call to GetListboxBody happens
on a null com ptr.
The related crash happens after
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#8613
casts a random frame (well I suppose it depends on whatever I happened to make
the parent of my listitem). It then tries to call OnContentRemoved on that frame
which tries to call RemoveMappingsForFrameSubtree but its mFrameConstructor
pointer is invalid so that when RemoveMappingsForFrameSubtree tries to use
mTempFrameTreeState the memory doesn't exist.

Comment 10

[[e-mail]]

Attachment: Possible fix

Based on my analysis I wrote this patch to fix the crash.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146312 [details] [diff] [review]
Possible fix

Oh, I see.  So the problem was that there was a listitem that was misplaced in
the DOM and then all hell broke loose?	<sigh>

OK, could you comment why you don't need to release listboxBody (or just make
it an nsCOMPtr; I doubt it asserts)?

Also, change the up-front check to make sure that the kid is also XUL and is a
listitem.  We really don't want to be running this code any time something is
inserted into some random XUL element... I assume we aren't checking the tag of
aContainer because XBL may be extending things and changing tagnames or
something?

r+sr=me with those issues fixed.

Comment 12

[[e-mail]]

(In reply to comment #11)
>OK, could you comment why you don't need to release listboxBody (or just make
>it an nsCOMPtr; I doubt it asserts)?
If I do that I can't seem to cast it, even with a .get()

Comment 13

[[e-mail]]

Attachment: Addressed comments

I added listbox/listitem checks, I added printfs to check all my test examples
and they were always listbox/listitem in the normal case.
I added a commented-out NS_RELEASE because of the static cast issue.
Oh, and so the reporter knows why the code worked if he generated incompatible
RDF first, I believe that the rebuild bypasses this code.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146341 [details] [diff] [review]
Addressed comments

r=bzbarsky, but hyatt really needs to look at this, since I'm not sure what
assumptions this code can make about tagnames (see the ResolveTag() below
that).

Drop him a mail at [e-mail] please?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146341 [details] [diff] [review]
Addressed comments

eh, forget it. sr=bzbarsky too.

Comment 16

[[e-mail]]

Fix checked in.

I see nsCSSFrameConstructor gets changed alot - 13 revs since patch ;-)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well, there's a lot of logic in that file (some only peripherally related to
frame construction, actually).... All that code's gotta have bugs, right?  ;)