Last Comment Bug 433429 - Crash [@ nsListBoxBodyFrame::OnContentRemoved]
: Crash [@ nsListBoxBodyFrame::OnContentRemoved]
Status: VERIFIED FIXED
: [sg:critical?]
: crash, testcase, verified1.9.0.4
Product: Core
Classification: Components
Component: XUL
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: xptoolkit.widgets
:
:
: 348483 417699
  Show dependency treegraph
 
Reported: 2008-05-12 17:06 PDT by Jesse Ruderman
Modified: 2008-12-02 08:46 PST (History)
10 users (show)
samuel.sidler+old: wanted1.9.0.x+
samuel.sidler+old: wanted1.8.1.x-
samuel.sidler+old: in‑testsuite?
See Also:
Crash Signature:
[@ nsListBoxBodyFrame::OnContentRemoved]


Attachments
testcase (crashes Firefox when loaded) (1.21 KB, application/vnd.mozilla.xul+xml)
2008-05-12 17:06 PDT, Jesse Ruderman
no flags Details
stack trace (8.55 KB, text/plain)
2008-05-12 17:06 PDT, Jesse Ruderman
no flags Details
ensure that only one box object is bound to listboxbodyframe (4.57 KB, patch)
2008-08-18 10:08 PDT, Olli Pettay [:smaug]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.9.0.4+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-05-12 17:06:09 PDT
Created attachment 320623 [details]
testcase (crashes Firefox when loaded)

Loading the testcase makes Mac trunk debug Firefox crash [@ nsListBoxBodyFrame::OnContentRemoved].  mContent and mRowCount are both 0xdddddddd, so I'm guessing |this| is a deleted nsListBoxBodyFrame.
Comment 1 Jesse Ruderman 2008-05-12 17:06:30 PDT
Created attachment 320624 [details]
stack trace
Comment 2 Boris Zbarsky (:bz) 2008-05-12 21:55:40 PDT
We're ending up with an nsListBoxObject that somehow holds a pointer to a dead mListBoxBody.  At a guess, we have multiple nsListBoxObjects for the same node.  Or something.  Because nsListBoxBodyFrame::Destroy notifies about itself going away...
Comment 3 Olli Pettay [:smaug] 2008-05-13 02:03:23 PDT
Can't reproduce on Linux.
Error console shows:
Security Error: Content at https://bugzilla.mozilla.org/attachment.cgi?id=320623 may not load data from data:text/xml,....
Comment 4 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-05-13 04:27:14 PDT
There is a pref that enables data urls for xbl, see discussion in bug 379644. I don't know the pref's name.
Comment 5 Jesse Ruderman 2008-05-13 11:23:21 PDT
Sorry, I forgot to mention that you have to set layout.debug.enable_data_xbl to true for the testcase to "work".  (See bug 379959.)
Comment 6 Boris Zbarsky (:bz) 2008-07-25 15:18:06 PDT
So we have nsListBoxObjects for two different <listbox>es, but one of them seems to find the listboxbody of the other one.  Should FindBodyContent bail out if it runs into a listbox?  roc, you know this code, right?
Comment 7 Robert O'Callahan (:roc) (Mozilla Corporation) 2008-07-25 15:48:22 PDT
I don't, really.
Comment 8 Olli Pettay [:smaug] 2008-08-18 10:08:54 PDT
Created attachment 334304 [details] [review]
ensure that only one box object is bound to listboxbodyframe

IMO this is enough. Just ensure that there is one-to-one mapping between
boxobject and frame. If someone creates strange XUL where there are several
listbox elements but only one listboxbody, the first created listboxobject gets access to the frame - sort of random, but we can't support that kind of XUL structure anyway. So better to prevent the crash.
Comment 9 Boris Zbarsky (:bz) 2008-08-19 10:48:10 PDT
Comment on attachment 334304 [details] [review]
ensure that only one box object is bound to listboxbodyframe

Can we check GetType() too, just to be safe?  Just static-casting interface pointers like that worries me.
Comment 10 Olli Pettay [:smaug] 2008-08-19 11:46:21 PDT
Well, if would be very strange if any other frame implemented nsIListBoxObject.
The whole setup is a bit strange; nsListBoxObject implements nsIListBoxObject
and so does nsListBoxBodyFrame. The idea is just that nsListBoxObject can forward
most of the method calls easily to nsListBoxBodyFrame.
Comment 11 Olli Pettay [:smaug] 2008-08-24 08:17:20 PDT
Pushed the patch
Comment 12 Olli Pettay [:smaug] 2008-08-24 08:17:47 PDT
Comment on attachment 334304 [details] [review]
ensure that only one box object is bound to listboxbodyframe

Is this too late for 1.9.0.2?
Comment 13 Olli Pettay [:smaug] 2008-08-24 08:20:15 PDT
The testcase WFM on 1.8.
Comment 14 Samuel Sidler (old account; do not CC) 2008-08-24 22:16:18 PDT
Comment on attachment 334304 [details] [review]
ensure that only one box object is bound to listboxbodyframe

Technically not too late, but I'd rather get this in 1.9.0.3 so it has more time to bake.

We should also get a crash test for this landed after it lands on the branch.
Comment 15 Daniel Veditz 2008-09-24 15:30:11 PDT
Comment on attachment 334304 [details] [review]
ensure that only one box object is bound to listboxbodyframe

Approved for 1.9.0.4, a=dveditz for release-drivers
Comment 16 Al Billings [:abillings] 2008-10-23 17:37:19 PDT
This is still reproducing in the current 1.9.0.4 build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102304 GranParadiso/3.0.4pre). Did this actually get checked in?
Comment 17 Olli Pettay [:smaug] 2008-10-23 17:43:19 PDT
Checked in 2008-10-23 10:10, do you have the right build?
Comment 18 Al Billings [:abillings] 2008-10-23 17:48:43 PDT
No, probably not. :-)

I *assumed* that it had been checked in during September so I didn't even check. I'll check with tomorrow's daily. Thanks!
Comment 19 Al Billings [:abillings] 2008-10-27 17:27:35 PDT
Verified for 1.9.0.4 with  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102704 GranParadiso/3.0.4pre.

Verified for trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081026 Minefield/3.1b2pre.
Comment 20 Jesse Ruderman 2008-11-29 18:48:48 PST
This test uses data: URLs for XBL bindings, so I think it can't be added to the crashtest suite as-is.
Comment 21 Boris Zbarsky (:bz) 2008-12-02 08:46:33 PST
It could be added to mochitest, right?

Or heck, it could use relative URLs for the bindings... would that not work in reftest?

Note You need to log in before you can comment on or make changes to this bug.