Last Comment Bug 378670 - Crash [@ nsListControlFrame::GetSelectedIndex] with popupshowing event and removing select
: Crash [@ nsListControlFrame::GetSelectedIndex] with popupshowing event and re...
Status: VERIFIED FIXED
[sg:critical?] using freed frame
: crash, testcase, verified1.8.0.14, verified1.8.1.8
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla1.9alpha5
Assigned To: Mats Palmgren (:mats)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-24 15:12 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2009-04-24 10:48 PDT (History)
12 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.14+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (443 bytes, text/html)
2007-04-24 15:12 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase2 (493 bytes, text/html)
2007-04-24 15:37 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch rev. 1 (21.32 KB, patch)
2007-04-25 10:33 PDT, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review
Patch rev. 2 (22.21 KB, patch)
2007-04-26 16:48 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 3 (17.76 KB, patch)
2007-05-09 08:03 PDT, Mats Palmgren (:mats)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch rev. 3.1 (rev. 3 with nits fixed) (48.91 KB, patch)
2007-05-12 09:48 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Patch rev. 3.1 -- branch version (53.34 KB, patch)
2007-07-09 18:21 PDT, Mats Palmgren (:mats)
mtschrep: approval1.8.1.5-
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.13-
dveditz: approval1.8.0.14+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-24 15:12:49 PDT
Created attachment 262691 [details]
testcase

See testcase, when clicking on the select, Mozilla crashes.
It also crashes on branch, so I'm marking it security sensitive (feel free to open it up, if not necessary).

Maybe this would be fixed with the fix for bug 279703

Talkback ID: TB31508003W
nsListControlFrame::GetSelectedIndex  [mozilla/layout/forms/nslistcontrolframe.cpp, line 1425]
nsListControlFrame::AboutToDropDown  [mozilla/layout/forms/nslistcontrolframe.cpp, line 1760]
nsComboboxControlFrame::ToggleList  [mozilla/layout/forms/nscomboboxcontrolframe.cpp, line 741]
nsListEventListener::MouseDown  [mozilla/layout/forms/nslistcontrolframe.cpp, line 2861]
nsEventTargetChainItem::HandleEvent  [mozilla/content/events/src/nseventdispatcher.cpp, line 209]
nsEventTargetChainItem::HandleEventTargetChain  [mozilla/content/events/src/nseventdispatcher.cpp, line 267]
nsEventDispatcher::Dispatch  [mozilla/content/events/src/nseventdispatcher.cpp, line 484]
PresShell::HandleEventInternal  [mozilla/layout/base/nspresshell.cpp, line 5924]
PresShell::HandlePositionedEvent  [mozilla/layout/base/nspresshell.cpp, line 5815]
PresShell::HandleEvent  [mozilla/layout/base/nspresshell.cpp, line 5658]
nsViewManager::HandleEvent  [mozilla/view/src/nsviewmanager.cpp, line 1457]
nsViewManager::DispatchEvent  [mozilla/view/src/nsviewmanager.cpp, line 1410]
HandleEvent  [mozilla/view/src/nsview.cpp, line 174]
nsWindow::DispatchEvent  [mozilla/widget/src/windows/nswindow.cpp, line 1107]
nsWindow::DispatchMouseEvent  [mozilla/widget/src/windows/nswindow.cpp, line 6288]
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-24 15:37:40 PDT
Created attachment 262693 [details]
testcase2

Mozilla also crashes with the popuphiding event on the select.
I don't get a talkback ID, I'm seeing something like a 'virtual function call' or something.
Comment 2 Mats Palmgren (:mats) 2007-04-25 08:33:45 PDT
> I'm seeing something like a 'virtual function call'

Which means executing random memory, if I'm not mistaken.

Patch coming up...
Comment 3 Mats Palmgren (:mats) 2007-04-25 10:33:25 PDT
Created attachment 262779 [details] [diff] [review]
Patch rev. 1

This patch also works for branch with minor modifications.
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2007-04-26 01:26:34 PDT
Comment on attachment 262779 [details] [diff] [review]
Patch rev. 1


Ugh, popup handling sucks. It is content objects which should fire
popupshowing/hiding events. But for branches this is the only thing we can do, I think.
And for trunk too until someone comes up with better solution.

>   // Don't flush anything but reflows lest it destroy us
>-  aPresContext->PresShell()->
>-    GetDocument()->FlushPendingNotifications(Flush_OnlyReflow);
>+  shell->GetDocument()->FlushPendingNotifications(Flush_OnlyReflow);
>+  NS_ASSERTION(weakFrame.IsAlive(), "Flush_OnlyReflow killed us");

I'd make this ENSURE_TRUE(weakFrame.IsAlive());
just to be sure. r=me, though I'm not an official layout/ reviewer.


(If we don't want to move all the event handling to content/ (trunk/moz2)
 I think we need something like "event-state-list" or whatever to call it.
 A list of events which should be dispatched before showing the popup and 
 after showing the popup. The list would have a strong ref to content object and
 after dispatching each event, before touching any nsIFrame objects, it would make sure
 that those are still alive, or in other words, it shouldn't keep any references to nsIFrames.)
Comment 5 Daniel Veditz [:dveditz] 2007-04-26 16:24:27 PDT
This is really too late to "block" the release, although of course it would be nice to get in. Unfortunately if we wait for every known security bug to be fixed we'll never be done.

If you can get amazing turnaround on layout moa= or sr= and get module owner to agree this isn't risky on the branch then maybe we can get it in, otherwise it looks safer to push an untested patch this size into the next release.
Comment 6 Mats Palmgren (:mats) 2007-04-26 16:48:41 PDT
Created attachment 262961 [details] [diff] [review]
Patch rev. 2

I changed the NS_ASSERTION to:

  if (!weakFrame.IsAlive()) {
    NS_ERROR("Flush_OnlyReflow destroyed the frame");
    return PR_FALSE;
  }

I didn't want to use ENSURE since those macros just warns.

Also, one other (unrelated) change:

>    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent)
>    {
>      mComboBox->ShowDropDown(!mComboBox->IsDroppedDown());
> -    return PR_FALSE;
> +    return NS_OK;
>    }

other than that it's the same as rev. 1
Comment 7 Boris Zbarsky [:bz] 2007-04-26 16:54:10 PDT
I'm not going to be able to do this review in an expeditious fashion (most likely not before end of next week at best)...
Comment 8 Boris Zbarsky [:bz] 2007-05-06 17:08:23 PDT
So on trunk FireMenuItemActiveEvent() is async, no?  Could we not do that on branch?
Comment 9 Aaron Leventhal 2007-05-07 18:38:05 PDT
It already is asynch on branch. I remember fixing it on both and that's verified by looking at lxr for the 1.8 branch. It calls nsFrame:FireDOMEvent which uses a PLDOMEvent.
Comment 10 Boris Zbarsky [:bz] 2007-05-07 19:13:23 PDT
Mats, why are all those changes around the event dispatch needed then?
Comment 11 Mats Palmgren (:mats) 2007-05-09 08:03:36 PDT
Created attachment 264237 [details] [diff] [review]
Patch rev. 3

The naming of FireMenuItemActiveEvent/FireDOMEvent lead me to believe
they were synchronous.  I really think we should rename them, together with
nsPLDOMEvent:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/public/nsPLDOMEvent.h&rev=1.3&root=/cvsroot&mark=52-54#46

Sorry for the misunderstanding.  This is the same patch without the
guards around FireMenuItemActiveEvent().
Comment 12 Boris Zbarsky [:bz] 2007-05-09 15:39:59 PDT
Comment on attachment 264237 [details] [diff] [review]
Patch rev. 3

>Index: layout/forms/nsComboboxControlFrame.cpp
>+      mListControlFrame->ComboboxFinish(mDisplayedIndex); // might destroy us

Mention that this would happen via ShowPopup()?
> nsComboboxControlFrame::ShowDropDown(PRBool aDoDropDown) 
>+    ShowList(PresContext(), (PR_FALSE == mDroppedDown)); // might destroy us

Might be clearer as

  ShowList(PresContext(), !mDroppedDown);

maybe?  Or even better aDoDropDown?

>Index: layout/forms/nsComboboxControlFrame.h

It might be nice to document which methods might destroy |this| or other frames.

You could probably make use of the new retval from ShowList() in the callers of ShowDropDown(), but it's fine like this too.

>Index: layout/forms/nsListControlFrame.cpp

>     case nsIDOMKeyEvent::DOM_VK_ESCAPE: {
>+      nsWeakFrame weakFrame(this);
>       AboutToRollup();
>-      } break;
>+      if (!weakFrame.IsAlive()) {
>+        aKeyEvent->PreventDefault();

Why the PreventDefault() here?  At least document, please.

Also document the functions in this class which can destroy frames?

r+sr=bzbarsky with those issues addressed.
Comment 13 Mats Palmgren (:mats) 2007-05-12 09:48:24 PDT
Created attachment 264608 [details] [diff] [review]
Patch rev. 3.1 (rev. 3 with nits fixed)

I added a lot of doc comments and noted which ones might destroy |this|
(which really should be read as "this method might destroy the entire
frame tree, pres shell and who knows what else").

> You could probably make use of the new retval from ShowList()
> in the callers of ShowDropDown(), but it's fine like this too.

I considered it, but I didn't want to change ShowDropDown()
since it's part of the nsIComboboxControlFrame interface.
This patch is still ok for branches.

> Why the PreventDefault() here?  At least document, please.

Due to the added early return we won't reach the one on line 2770:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsListControlFrame.cpp&rev=1.423&root=/cvsroot&mark=2640-2642,2770#2759

This patch only adds comment changes and the
ShowList(PresContext(), aDoDropDown) nit fix, so I'm assuming r+sr.
Comment 14 Boris Zbarsky [:bz] 2007-05-13 19:50:12 PDT
Yeah, sounds good.
Comment 15 Mats Palmgren (:mats) 2007-05-17 04:46:49 PDT
Checked in to trunk at 2007-05-17 04:12 PDT.

-> FIXED
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-05-17 08:18:02 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070517 Minefield/3.0a5pre
Comment 17 Daniel Veditz [:dveditz] 2007-07-09 15:26:41 PDT
Does this patch apply to the 1.8 branch as-is, or do we need to back-port it in order to land?
Comment 18 Mats Palmgren (:mats) 2007-07-09 18:21:17 PDT
Created attachment 271594 [details] [diff] [review]
Patch rev. 3.1 -- branch version

Fairly straightforward, a couple of minor adjustments was needed -
"aPresContext" instead of "PresContext()" and things of that nature.
A took the whole patch, comment changes and all, to make it easier to
see the differences.  You can diff the patches and tell me if you think
we need a second review on the branch version.
I have verified that it fixes both testcases on both branches (Linux).
I also (briefly) reviewed the 1.8 branch code to make sure the patch
is complete for branch too and I believe it is.
Comment 19 Mats Palmgren (:mats) 2007-07-09 18:25:30 PDT
Hmm, Bugzilla's patch diff isn't that good it seems.  Try downloading
the two patches and diff them manually instead.
Comment 20 Daniel Veditz [:dveditz] 2007-07-10 10:43:41 PDT
Comment on attachment 271594 [details] [diff] [review]
Patch rev. 3.1 -- branch version

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Comment 21 Mike Schroepfer 2007-07-11 12:40:10 PDT
Comment on attachment 271594 [details] [diff] [review]
Patch rev. 3.1 -- branch version

Pushing to 1.8.1.6 since we are closing out on 1.8.1.5
Comment 22 Daniel Veditz [:dveditz] 2007-08-07 11:34:50 PDT
Comment on attachment 271594 [details] [diff] [review]
Patch rev. 3.1 -- branch version

moving approval request to 1.8.0.14 to match the 1.8.1 branch landing
Comment 23 Daniel Veditz [:dveditz] 2007-08-21 14:55:04 PDT
Comment on attachment 271594 [details] [diff] [review]
Patch rev. 3.1 -- branch version

approved for 1.8.1.7 amd 1.8.0.14, a=dveditz for release-drivers
Comment 24 Mats Palmgren (:mats) 2007-08-30 18:55:51 PDT
MOZILLA_1_8_BRANCH
mozilla/layout/forms/nsComboboxControlFrame.cpp 	1.324.6.10
mozilla/layout/forms/nsComboboxControlFrame.h 	1.134.6.2
mozilla/layout/forms/nsListControlFrame.cpp 	1.372.2.7
mozilla/layout/forms/nsListControlFrame.h 	1.141.4.2 

MOZILLA_1_8_0_BRANCH
mozilla/layout/forms/nsComboboxControlFrame.cpp 	1.324.6.2.2.4
mozilla/layout/forms/nsComboboxControlFrame.h 	1.134.10.1
mozilla/layout/forms/nsListControlFrame.cpp 	1.372.2.1.4.4
mozilla/layout/forms/nsListControlFrame.h 	1.141.14.2 
Comment 25 Carsten Book [:Tomcat] - PTO-back Sept 4th 2007-09-03 14:56:15 PDT
verified fixed for 1.8.1.7 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/20070903 BonEcho/2.0.0.7pre ID:2007090304 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre)Gecko/2007090308 BonEcho/2.0.0.7pre

no crash on testcases - adding verified keyword
Comment 26 Al Billings [:abillings] 2007-12-10 17:54:35 PST
Verified for 1.8.0 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre. Crashes in the last Firefox 1.5.0.12 release but not with this build.
Comment 27 Bob Clary [:bc:] 2009-04-24 10:48:51 PDT
mochitest landed
http://hg.mozilla.org/mozilla-central/rev/4b9b8361e0dd

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