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

VERIFIED FIXED in mozilla1.9alpha5

Status

()

Core
Event Handling
--
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Mats Palmgren (vacation - back in August))

Tracking

(4 keywords)

unspecified
mozilla1.9alpha5
crash, testcase, verified1.8.0.14, verified1.8.1.8
Points:
---
Bug Flags:
blocking1.8.1.8 +
wanted1.8.1.x +
blocking1.8.0.14 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] using freed frame, crash signature)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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]
(Reporter)

Comment 1

11 years ago
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.
> I'm seeing something like a 'virtual function call'

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

Patch coming up...
Assignee: events → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Whiteboard: [sg:critical?] using freed frame
Created attachment 262779 [details] [diff] [review]
Patch rev. 1

This patch also works for branch with minor modifications.
Attachment #262779 - Flags: review?(Olli.Pettay)

Comment 4

11 years ago
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.)
Attachment #262779 - Flags: review?(Olli.Pettay) → review+
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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.12?
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
Attachment #262779 - Attachment is obsolete: true
Attachment #262961 - Flags: superreview?(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)...
So on trunk FireMenuItemActiveEvent() is async, no?  Could we not do that on branch?

Comment 9

10 years ago
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.
Mats, why are all those changes around the event dispatch needed then?
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().
Attachment #262961 - Attachment is obsolete: true
Attachment #264237 - Flags: superreview?(bzbarsky)
Attachment #264237 - Flags: review?(bzbarsky)
Attachment #262961 - Flags: superreview?(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.
Attachment #264237 - Flags: superreview?(bzbarsky)
Attachment #264237 - Flags: superreview+
Attachment #264237 - Flags: review?(bzbarsky)
Attachment #264237 - Flags: review+
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.
Yeah, sounds good.
Checked in to trunk at 2007-05-17 04:12 PDT.

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha5
Version: Trunk → unspecified
(Reporter)

Comment 16

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070517 Minefield/3.0a5pre
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Does this patch apply to the 1.8 branch as-is, or do we need to back-port it in order to land?
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.
Attachment #271594 - Flags: approval1.8.1.5?
Attachment #271594 - Flags: approval1.8.0.13?
Hmm, Bugzilla's patch diff isn't that good it seems.  Try downloading
the two patches and diff them manually instead.
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
Attachment #271594 - Flags: approval1.8.1.5?
Attachment #271594 - Flags: approval1.8.1.5+
Attachment #271594 - Flags: approval1.8.0.13?
Attachment #271594 - Flags: approval1.8.0.13+

Comment 21

10 years ago
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
Attachment #271594 - Flags: approval1.8.1.6?
Attachment #271594 - Flags: approval1.8.1.5-
Attachment #271594 - Flags: approval1.8.1.5+
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
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
Attachment #271594 - Flags: approval1.8.0.14?
Attachment #271594 - Flags: approval1.8.0.13-
Attachment #271594 - Flags: approval1.8.0.13+
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
Attachment #271594 - Flags: approval1.8.1.7?
Attachment #271594 - Flags: approval1.8.1.7+
Attachment #271594 - Flags: approval1.8.0.14?
Attachment #271594 - Flags: approval1.8.0.14+
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 
Keywords: fixed1.8.0.14, fixed1.8.1.7
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
Keywords: fixed1.8.1.7 → verified1.8.1.7
Group: security
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.
Keywords: fixed1.8.0.14 → verified1.8.0.14

Comment 27

8 years ago
mochitest landed
http://hg.mozilla.org/mozilla-central/rev/4b9b8361e0dd
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsListControlFrame::GetSelectedIndex]
You need to log in before you can comment on or make changes to this bug.