378670 - Crash [@ nsListControlFrame::GetSelectedIndex] with popupshowing event and removing select

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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 (dead)]

Attachment: 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 (inactive)]

> 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 (inactive)]

Attachment: Patch rev. 1

This patch also works for branch with minor modifications.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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]]

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 (inactive)]

Attachment: 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 [:bzbarsky]]

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 [:bzbarsky]]

So on trunk FireMenuItemActiveEvent() is async, no?  Could we not do that on branch?

Comment 9

[Aaron Leventhal]

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 [:bzbarsky]]

Mats, why are all those changes around the event dispatch needed then?

Comment 11

[Mats Palmgren (inactive)]

Attachment: 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 [:bzbarsky]]

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 (inactive)]

Attachment: 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 [:bzbarsky]]

Yeah, sounds good.

Comment 15

[Mats Palmgren (inactive)]

Checked in to trunk at 2007-05-17 04:12 PDT.

-> FIXED

Comment 16

[Martijn Wargers (dead)]

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]]

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 (inactive)]

Attachment: 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 (inactive)]

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]]

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]

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]]

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]]

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 (inactive)]

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]]

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 - ex-MoCo]]

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] (inactive)]

mochitest landed
http://hg.mozilla.org/mozilla-central/rev/4b9b8361e0dd