Protect against frame destruction during event handling and document when that can occur. b=378670 r+sr=bzbarsky
authormats.palmgren@bredband.net
Thu, 17 May 2007 04:12:30 -0700
changeset 1564 0121fa857ecbfbd7c3fababab4361ab179281cfd
parent 1563 0a40cb0d4a24512b661120bb7904c4d0b80134fa
child 1565 29667149241005bb56ed2e551bc0664aa02c0e9d
push idunknown
push userunknown
push dateunknown
bugs378670
milestone1.9a5pre
Protect against frame destruction during event handling and document when that can occur. b=378670 r+sr=bzbarsky
layout/forms/nsComboboxControlFrame.cpp
layout/forms/nsComboboxControlFrame.h
layout/forms/nsListControlFrame.cpp
layout/forms/nsListControlFrame.h
--- a/layout/forms/nsComboboxControlFrame.cpp
+++ b/layout/forms/nsComboboxControlFrame.cpp
@@ -135,17 +135,17 @@ class nsComboButtonListener: public nsID
   NS_IMETHOD MouseUp(nsIDOMEvent* aMouseEvent) { return PR_FALSE; }
   NS_IMETHOD MouseDblClick(nsIDOMEvent* aMouseEvent) { return PR_FALSE; }
   NS_IMETHOD MouseOver(nsIDOMEvent* aMouseEvent) { return PR_FALSE; }
   NS_IMETHOD MouseOut(nsIDOMEvent* aMouseEvent) { return PR_FALSE; }
 
   NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent) 
   {
     mComboBox->ShowDropDown(!mComboBox->IsDroppedDown());
-    return PR_FALSE; 
+    return NS_OK; 
   }
 
   nsComboButtonListener(nsComboboxControlFrame* aCombobox) 
   { 
     mComboBox = aCombobox; 
   }
 
   virtual ~nsComboButtonListener() {}
@@ -341,17 +341,20 @@ nsComboboxControlFrame::SetFocus(PRBool 
 {
   nsWeakFrame weakFrame(this);
   if (aOn) {
     nsListControlFrame::ComboboxFocusSet();
     mFocused = this;
   } else {
     mFocused = nsnull;
     if (mDroppedDown) {
-      mListControlFrame->ComboboxFinish(mDisplayedIndex);
+      mListControlFrame->ComboboxFinish(mDisplayedIndex); // might destroy us
+      if (!weakFrame.IsAlive()) {
+        return;
+      }
     }
     // May delete |this|.
     mListControlFrame->FireOnChange();
   }
 
   if (!weakFrame.IsAlive()) {
     return;
   }
@@ -394,54 +397,55 @@ nsComboboxControlFrame::ShowPopup(PRBool
                      NS_XUL_POPUP_SHOWING : NS_XUL_POPUP_HIDING, nsnull,
                      nsMouseEvent::eReal);
 
   nsCOMPtr<nsIPresShell> shell = PresContext()->GetPresShell();
   if (shell) 
     shell->HandleDOMEventWithTarget(mContent, &event, &status);
 }
 
-// Show the dropdown list
-
-void 
+PRBool
 nsComboboxControlFrame::ShowList(nsPresContext* aPresContext, PRBool aShowList)
 {
-  nsIWidget* widget = nsnull;
+  nsCOMPtr<nsIPresShell> shell = PresContext()->GetPresShell();
+
+  nsWeakFrame weakFrame(this);
+  ShowPopup(aShowList);  // might destroy us
+  if (!weakFrame.IsAlive()) {
+    return PR_FALSE;
+  }
 
-  // Get parent view
-  nsIFrame * listFrame;
-  if (NS_OK == mListControlFrame->QueryInterface(NS_GET_IID(nsIFrame), (void **)&listFrame)) {
+  mDroppedDown = aShowList;
+  if (mDroppedDown) {
+    // The listcontrol frame will call back to the nsComboboxControlFrame's
+    // ListWasSelected which will stop the capture.
+    mListControlFrame->AboutToDropDown();
+    mListControlFrame->CaptureMouseEvents(PR_TRUE);
+  }
+
+  // Don't flush anything but reflows lest it destroy us
+  shell->GetDocument()->FlushPendingNotifications(Flush_OnlyReflow);
+  if (!weakFrame.IsAlive()) {
+    NS_ERROR("Flush_OnlyReflow destroyed the frame");
+    return PR_FALSE;
+  }
+
+  nsIFrame* listFrame;
+  CallQueryInterface(mListControlFrame, &listFrame);
+  if (listFrame) {
     nsIView* view = listFrame->GetView();
     NS_ASSERTION(view, "nsComboboxControlFrame view is null");
     if (view) {
-    	widget = view->GetWidget();
+      nsIWidget* widget = view->GetWidget();
+      if (widget)
+        widget->CaptureRollupEvents(this, mDroppedDown, mDroppedDown);
     }
   }
 
-  if (PR_TRUE == aShowList) {
-    ShowPopup(PR_TRUE);
-    mDroppedDown = PR_TRUE;
-
-     // The listcontrol frame will call back to the nsComboboxControlFrame's ListWasSelected
-     // which will stop the capture.
-    mListControlFrame->AboutToDropDown();
-    mListControlFrame->CaptureMouseEvents(PR_TRUE);
-
-  } else {
-    ShowPopup(PR_FALSE);
-    mDroppedDown = PR_FALSE;
-  }
-
-  // Don't flush anything but reflows lest it destroy us
-  aPresContext->PresShell()->
-    GetDocument()->FlushPendingNotifications(Flush_OnlyReflow);
-
-  if (widget)
-    widget->CaptureRollupEvents((nsIRollupListener *)this, mDroppedDown, aShowList);
-
+  return weakFrame.IsAlive();
 }
 
 nsresult
 nsComboboxControlFrame::ReflowDropdown(nsPresContext*  aPresContext, 
                                        const nsHTMLReflowState& aReflowState)
 {
   // All we want out of it later on, really, is the height of a row, so we
   // don't even need to cache mDropdownFrame's ascent or anything.  If we don't
@@ -731,19 +735,19 @@ nsComboboxControlFrame::ShowDropDown(PRB
   if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::disabled)) {
     return;
   }
 
   if (!mDroppedDown && aDoDropDown) {
     if (mListControlFrame) {
       mListControlFrame->SyncViewWithFrame();
     }
-    ToggleList(PresContext());
+    ShowList(PresContext(), aDoDropDown); // might destroy us
   } else if (mDroppedDown && !aDoDropDown) {
-    ToggleList(PresContext());
+    ShowList(PresContext(), aDoDropDown); // might destroy us
   }
 }
 
 void
 nsComboboxControlFrame::SetDropDown(nsIFrame* aDropDownFrame)
 {
   mDropdownFrame = aDropDownFrame;
  
@@ -751,26 +755,16 @@ nsComboboxControlFrame::SetDropDown(nsIF
 }
 
 nsIFrame*
 nsComboboxControlFrame::GetDropDown() 
 {
   return mDropdownFrame;
 }
 
-// Toggle dropdown list.
-
-NS_IMETHODIMP 
-nsComboboxControlFrame::ToggleList(nsPresContext* aPresContext)
-{
-  ShowList(aPresContext, (PR_FALSE == mDroppedDown));
-
-  return NS_OK;
-}
-
 ///////////////////////////////////////////////////////////////
 
 NS_IMETHODIMP
 nsComboboxControlFrame::RedisplaySelectedText()
 {
   return RedisplayText(mListControlFrame->GetSelectedIndex());
 }
 
@@ -1188,17 +1182,17 @@ nsComboboxControlFrame::Destroy()
     // Get parent view
     nsIFrame * listFrame;
     if (NS_OK == mListControlFrame->QueryInterface(NS_GET_IID(nsIFrame), (void **)&listFrame)) {
       nsIView* view = listFrame->GetView();
       NS_ASSERTION(view, "nsComboboxControlFrame view is null");
       if (view) {
         nsIWidget* widget = view->GetWidget();
         if (widget)
-          widget->CaptureRollupEvents((nsIRollupListener *)this, PR_FALSE, PR_TRUE);
+          widget->CaptureRollupEvents(this, PR_FALSE, PR_TRUE);
       }
     }
   }
 
   // Cleanup frames in popup child list
   mPopupFrames.DestroyFrames();
   nsContentUtils::DestroyAnonymousContent(&mDisplayContent);
   nsContentUtils::DestroyAnonymousContent(&mButtonContent);
@@ -1257,30 +1251,33 @@ nsComboboxControlFrame::GetAdditionalChi
 
 //----------------------------------------------------------------------
   //nsIRollupListener
 //----------------------------------------------------------------------
 NS_IMETHODIMP 
 nsComboboxControlFrame::Rollup()
 {
   if (mDroppedDown) {
-    mListControlFrame->AboutToRollup();
-    ShowDropDown(PR_FALSE);
+    nsWeakFrame weakFrame(this);
+    mListControlFrame->AboutToRollup(); // might destroy us
+    if (!weakFrame.IsAlive())
+      return NS_OK;
+    ShowDropDown(PR_FALSE); // might destroy us
+    if (!weakFrame.IsAlive())
+      return NS_OK;
     mListControlFrame->CaptureMouseEvents(PR_FALSE);
   }
   return NS_OK;
 }
 
 void
 nsComboboxControlFrame::RollupFromList()
 {
-  nsPresContext* aPresContext = PresContext();
-
-  ShowList(aPresContext, PR_FALSE);
-  mListControlFrame->CaptureMouseEvents(PR_FALSE);
+  if (ShowList(PresContext(), PR_FALSE))
+    mListControlFrame->CaptureMouseEvents(PR_FALSE);
 }
 
 PRInt32
 nsComboboxControlFrame::UpdateRecentIndex(PRInt32 aIndex)
 {
   PRInt32 index = mRecentSelectedIndex;
   if (mRecentSelectedIndex == NS_SKIP_NOTIFY_INDEX || aIndex == NS_SKIP_NOTIFY_INDEX)
     mRecentSelectedIndex = aIndex;
--- a/layout/forms/nsComboboxControlFrame.h
+++ b/layout/forms/nsComboboxControlFrame.h
@@ -146,23 +146,37 @@ public:
                                  nsIFrame*       aChildList);
   virtual nsIAtom* GetAdditionalChildListName(PRInt32 aIndex) const;
 
   virtual nsIFrame* GetContentInsertionFrame();
 
   // nsIFormControlFrame
   virtual nsresult SetFormProperty(nsIAtom* aName, const nsAString& aValue);
   virtual nsresult GetFormProperty(nsIAtom* aName, nsAString& aValue) const; 
+  /**
+   * Inform the control that it got (or lost) focus.
+   * If it lost focus, the dropdown menu will be rolled up if needed,
+   * and FireOnChange() will be called.
+   * @param aOn PR_TRUE if got focus, PR_FALSE if lost focus.
+   * @param aRepaint if PR_TRUE then force repaint (NOTE: we always force repaint currently)
+   * @note This method might destroy |this|.
+   */
   virtual void SetFocus(PRBool aOn, PRBool aRepaint);
 
   //nsIComboboxControlFrame
   virtual PRBool IsDroppedDown() { return mDroppedDown; }
+  /**
+   * @note This method might destroy |this|.
+   */
   virtual void ShowDropDown(PRBool aDoDropDown);
   virtual nsIFrame* GetDropDown();
   virtual void SetDropDown(nsIFrame* aDropDownFrame);
+  /**
+   * @note This method might destroy |this|.
+   */
   virtual void RollupFromList();
   virtual void AbsolutelyPositionDropDown();
   virtual PRInt32 GetIndexOfDisplayArea();
   NS_IMETHOD RedisplaySelectedText();
   virtual PRInt32 UpdateRecentIndex(PRInt32 aIndex);
   virtual void OnContentReset();
 
   // nsISelectControlFrame
@@ -171,26 +185,32 @@ public:
   NS_IMETHOD GetOptionSelected(PRInt32 aIndex, PRBool* aValue);
   NS_IMETHOD DoneAddingChildren(PRBool aIsDone);
   NS_IMETHOD OnOptionSelected(nsPresContext* aPresContext,
                               PRInt32 aIndex,
                               PRBool aSelected);
   NS_IMETHOD OnSetSelectedIndex(PRInt32 aOldIndex, PRInt32 aNewIndex);
 
   //nsIRollupListener
-  // NS_DECL_NSIROLLUPLISTENER
+  /**
+   * Hide the dropdown menu and stop capturing mouse events.
+   * @note This method might destroy |this|.
+   */
   NS_IMETHOD Rollup();
-   // a combobox should roll up if a mousewheel event happens outside of
-   // the popup area
+  /**
+   * A combobox should roll up if a mousewheel event happens outside of
+   * the popup area.
+   */
   NS_IMETHOD ShouldRollupOnMouseWheelEvent(PRBool *aShouldRollup)
     { *aShouldRollup = PR_TRUE; return NS_OK;}
-  //NS_IMETHOD ShouldRollupOnMouseWheelEvent(nsIWidget *aWidget, PRBool *aShouldRollup) 
-  //{ *aShouldRollup = PR_FALSE; return NS_OK;}
 
-  // a combobox should not roll up if activated by a mouse activate message (eg. X-mouse)
+  /**
+   * A combobox should not roll up if activated by a mouse activate message
+   * (eg. X-mouse).
+   */
   NS_IMETHOD ShouldRollupOnMouseActivate(PRBool *aShouldRollup)
     { *aShouldRollup = PR_FALSE; return NS_OK;}
 
   // nsIScrollableViewProvider
   virtual nsIScrollableView* GetScrollableView();
 
   //nsIStatefulFrame
   NS_IMETHOD SaveState(SpecialStateID aStateID, nsPresState** aState);
@@ -212,24 +232,34 @@ protected:
   public:
     NS_DECL_NSIRUNNABLE
     RedisplayTextEvent(nsComboboxControlFrame *c) : mControlFrame(c) {}
     void Revoke() { mControlFrame = nsnull; }
   private:
     nsComboboxControlFrame *mControlFrame;
   };
   
+  /**
+   * Show or hide the dropdown list.
+   * @note This method might destroy |this|.
+   */
   void ShowPopup(PRBool aShowPopup);
-  void ShowList(nsPresContext* aPresContext, PRBool aShowList);
+
+  /**
+   * Show or hide the dropdown list.
+   * @param aShowList PR_TRUE to show, PR_FALSE to hide the dropdown.
+   * @note This method might destroy |this|.
+   * @return PR_FALSE if this frame is destroyed, PR_TRUE if still alive.
+   */
+  PRBool ShowList(nsPresContext* aPresContext, PRBool aShowList);
   void CheckFireOnChange();
   void FireValueChangeEvent();
   nsresult RedisplayText(PRInt32 aIndex);
   void HandleRedisplayTextEvent();
   void ActuallyDisplayText(PRBool aNotify);
-  NS_IMETHOD ToggleList(nsPresContext* aPresContext);
 
   nsFrameList              mPopupFrames;             // additional named child list
   nsCOMPtr<nsIContent>     mDisplayContent;          // Anonymous content used to display the current selection
   nsCOMPtr<nsIContent>     mButtonContent;           // Anonymous content for the button
   nsIFrame*                mDisplayFrame;            // frame to display selection
   nsIFrame*                mButtonFrame;             // button frame
   nsIFrame*                mDropdownFrame;           // dropdown list frame
   nsIFrame*                mTextFrame;               // display area frame
--- a/layout/forms/nsListControlFrame.cpp
+++ b/layout/forms/nsListControlFrame.cpp
@@ -1136,20 +1136,16 @@ nsListControlFrame::Init(nsIContent*    
                                   NS_GET_IID(nsIDOMKeyListener));
 
   mStartSelectionIndex = kNothingSelected;
   mEndSelectionIndex = kNothingSelected;
 
   return result;
 }
 
-//---------------------------------------------------------
-// Returns whether the nsIDOMHTMLSelectElement supports 
-// mulitple selection
-//---------------------------------------------------------
 PRBool
 nsListControlFrame::GetMultiple(nsIDOMHTMLSelectElement* aSelect) const
 {
   PRBool multiple = PR_FALSE;
   nsresult rv = NS_OK;
   if (aSelect) {
     rv = aSelect->GetMultiple(&multiple);
   } else {
@@ -1161,73 +1157,57 @@ nsListControlFrame::GetMultiple(nsIDOMHT
     }
   }
   if (NS_SUCCEEDED(rv)) {
     return multiple;
   }
   return PR_FALSE;
 }
 
-
-//---------------------------------------------------------
-// Returns the nsIContent object in the collection 
-// for a given index (AddRefs)
-//---------------------------------------------------------
 already_AddRefed<nsIContent> 
 nsListControlFrame::GetOptionAsContent(nsIDOMHTMLOptionsCollection* aCollection, PRInt32 aIndex) 
 {
   nsIContent * content = nsnull;
   nsCOMPtr<nsIDOMHTMLOptionElement> optionElement = GetOption(aCollection,
                                                               aIndex);
 
   NS_ASSERTION(optionElement != nsnull, "could not get option element by index!");
 
   if (optionElement) {
     CallQueryInterface(optionElement, &content);
   }
  
   return content;
 }
 
-//---------------------------------------------------------
-// for a given index it returns the nsIContent object 
-// from the select
-//---------------------------------------------------------
 already_AddRefed<nsIContent> 
 nsListControlFrame::GetOptionContent(PRInt32 aIndex) const
   
 {
   nsCOMPtr<nsIDOMHTMLOptionsCollection> options = GetOptions(mContent);
   NS_ASSERTION(options.get() != nsnull, "Collection of options is null!");
 
   if (options) {
     return GetOptionAsContent(options, aIndex);
   } 
   return nsnull;
 }
 
-//---------------------------------------------------------
-// This returns the options collection for aContent, if any
-//---------------------------------------------------------
 already_AddRefed<nsIDOMHTMLOptionsCollection>
 nsListControlFrame::GetOptions(nsIContent * aContent)
 {
   nsIDOMHTMLOptionsCollection* options = nsnull;
   nsCOMPtr<nsIDOMHTMLSelectElement> selectElement = do_QueryInterface(aContent);
   if (selectElement) {
     selectElement->GetOptions(&options);  // AddRefs (1)
   }
 
   return options;
 }
 
-//---------------------------------------------------------
-// Returns the nsIDOMHTMLOptionElement for a given index 
-// in the select's collection
-//---------------------------------------------------------
 already_AddRefed<nsIDOMHTMLOptionElement>
 nsListControlFrame::GetOption(nsIDOMHTMLOptionsCollection* aCollection,
                               PRInt32 aIndex)
 {
   nsCOMPtr<nsIDOMNode> node;
   if (NS_SUCCEEDED(aCollection->Item(aIndex, getter_AddRefs(node)))) {
     NS_ASSERTION(node,
                  "Item was successful, but node from collection was null!");
@@ -1238,81 +1218,61 @@ nsListControlFrame::GetOption(nsIDOMHTML
       return option;
     }
   } else {
     NS_ERROR("Couldn't get option by index from collection!");
   }
   return nsnull;
 }
 
-
-//---------------------------------------------------------
-// For a given piece of content, it determines whether the 
-// content (an option) is selected or not
-// return PR_TRUE if it is, PR_FALSE if it is NOT
-//---------------------------------------------------------
 PRBool 
 nsListControlFrame::IsContentSelected(nsIContent* aContent) const
 {
   PRBool isSelected = PR_FALSE;
 
   nsCOMPtr<nsIDOMHTMLOptionElement> optEl = do_QueryInterface(aContent);
   if (optEl)
     optEl->GetSelected(&isSelected);
 
   return isSelected;
 }
 
-
-//---------------------------------------------------------
-// For a given index is return whether the content is selected
-//---------------------------------------------------------
 PRBool 
 nsListControlFrame::IsContentSelectedByIndex(PRInt32 aIndex) const 
 {
   nsCOMPtr<nsIContent> content = GetOptionContent(aIndex);
   NS_ASSERTION(content, "Failed to retrieve option content");
 
   return IsContentSelected(content);
 }
 
-//---------------------------------------------------------
-// gets the content (an option) by index and then set it as
-// being selected or not selected
-//---------------------------------------------------------
 NS_IMETHODIMP
 nsListControlFrame::OnOptionSelected(nsPresContext* aPresContext,
                                      PRInt32 aIndex,
                                      PRBool aSelected)
 {
   if (aSelected) {
     ScrollToIndex(aIndex);
   }
   return NS_OK;
 }
 
-//---------------------------------------------------------
 PRIntn
 nsListControlFrame::GetSkipSides() const
 {    
     // Don't skip any sides during border rendering
   return 0;
 }
 
-//---------------------------------------------------------
 void
 nsListControlFrame::OnContentReset()
 {
   ResetList(PR_TRUE);
 }
 
-//---------------------------------------------------------
-// Resets the select back to it's original default values;
-// those values as determined by the original HTML
-//---------------------------------------------------------
 void 
 nsListControlFrame::ResetList(PRBool aAllowScrolling)
 {
   // if all the frames aren't here 
   // don't bother reseting
   if (!mIsAllFramesHere) {
     return;
   }
@@ -1332,17 +1292,16 @@ nsListControlFrame::ResetList(PRBool aAl
   }
 
   mStartSelectionIndex = kNothingSelected;
   mEndSelectionIndex = kNothingSelected;
 
   // Combobox will redisplay itself with the OnOptionSelected event
 } 
  
-//---------------------------------------------------------
 void 
 nsListControlFrame::SetFocus(PRBool aOn, PRBool aRepaint)
 {
   if (aOn) {
     ComboboxFocusSet();
     mFocused = this;
   } else {
     mFocused = nsnull;
@@ -1352,31 +1311,24 @@ nsListControlFrame::SetFocus(PRBool aOn,
   Invalidate(nsRect(0,0,mRect.width,mRect.height), PR_TRUE);
 }
 
 void nsListControlFrame::ComboboxFocusSet()
 {
   gLastKeyTime = 0;
 }
 
-//---------------------------------------------------------
 void
 nsListControlFrame::SetComboboxFrame(nsIFrame* aComboboxFrame)
 {
   if (nsnull != aComboboxFrame) {
     aComboboxFrame->QueryInterface(NS_GET_IID(nsIComboboxControlFrame),(void**) &mComboboxFrame); 
   }
 }
 
-
-//---------------------------------------------------------
-// Gets the text of the currently selected item
-// if the there are zero items then an empty string is returned
-// if there is nothing selected, then the 0th item's text is returned
-//---------------------------------------------------------
 void
 nsListControlFrame::GetOptionText(PRInt32 aIndex, nsAString & aStr)
 {
   aStr.SetLength(0);
   nsCOMPtr<nsIDOMHTMLOptionsCollection> options = GetOptions(mContent);
 
   if (options) {
     PRUint32 numOptions;
@@ -1408,36 +1360,33 @@ nsListControlFrame::GetOptionText(PRInt3
 #else
         optionElement->GetText(aStr);
 #endif
       }
     }
   }
 }
 
-//---------------------------------------------------------
 PRInt32
 nsListControlFrame::GetSelectedIndex()
 {
   PRInt32 aIndex;
   
   nsCOMPtr<nsIDOMHTMLSelectElement> selectElement(do_QueryInterface(mContent));
   selectElement->GetSelectedIndex(&aIndex);
   
   return aIndex;
 }
 
-//---------------------------------------------------------
 PRBool 
 nsListControlFrame::IsInDropDownMode() const
 {
   return (mComboboxFrame != nsnull);
 }
 
-//---------------------------------------------------------
 PRInt32
 nsListControlFrame::GetNumberOfOptions()
 {
   if (mContent != nsnull) {
     nsCOMPtr<nsIDOMHTMLOptionsCollection> options = GetOptions(mContent);
 
     if (!options) {
       return 0;
@@ -1463,17 +1412,16 @@ PRBool nsListControlFrame::CheckIfAllFra
     // all the frames are there
     mIsAllFramesHere = PR_TRUE;//NS_OK == CountAllChild(node, numContentItems);
   }
   // now make sure we have a frame each piece of content
 
   return mIsAllFramesHere;
 }
 
-//-------------------------------------------------------------------
 NS_IMETHODIMP
 nsListControlFrame::DoneAddingChildren(PRBool aIsDone)
 {
   mIsAllContentHere = aIsDone;
   if (mIsAllContentHere) {
     // Here we check to see if all the frames have been created 
     // for all the content.
     // If so, then we can initialize;
@@ -1483,17 +1431,16 @@ nsListControlFrame::DoneAddingChildren(P
         mHasBeenInitialized = PR_TRUE;
         ResetList(PR_TRUE);
       }
     }
   }
   return NS_OK;
 }
 
-//-------------------------------------------------------------------
 NS_IMETHODIMP
 nsListControlFrame::AddOption(nsPresContext* aPresContext, PRInt32 aIndex)
 {
 #ifdef DO_REFLOW_DEBUG
   printf("---- Id: %d nsLCF %p Added Option %d\n", mReflowId, this, aIndex);
 #endif
 
   if (!mIsAllContentHere) {
@@ -1511,17 +1458,16 @@ nsListControlFrame::AddOption(nsPresCont
   }
 
   // Make sure we scroll to the selected option as needed
   mNeedToReset = PR_TRUE;
   mPostChildrenLoadedReset = mIsAllContentHere;
   return NS_OK;
 }
 
-//-------------------------------------------------------------------
 NS_IMETHODIMP
 nsListControlFrame::RemoveOption(nsPresContext* aPresContext, PRInt32 aIndex)
 {
   // Need to reset if we're a dropdown
   if (IsInDropDownMode()) {
     mNeedToReset = PR_TRUE;
     mPostChildrenLoadedReset = mIsAllContentHere;
   }
@@ -1628,17 +1574,17 @@ nsListControlFrame::ComboboxFinish(PRInt
     PerformSelection(aIndex, PR_FALSE, PR_FALSE);
 
     PRInt32 displayIndex = mComboboxFrame->GetIndexOfDisplayArea();
 
     if (displayIndex != aIndex) {
       mComboboxFrame->RedisplaySelectedText();
     }
 
-    mComboboxFrame->RollupFromList();
+    mComboboxFrame->RollupFromList(); // might destroy us
   }
 }
 
 // Send out an onchange notification.
 void
 nsListControlFrame::FireOnChange()
 {
   if (mComboboxFrame) {
@@ -1657,17 +1603,16 @@ nsListControlFrame::FireOnChange()
   nsEvent event(PR_TRUE, NS_FORM_CHANGE);
 
   nsCOMPtr<nsIPresShell> presShell = PresContext()->GetPresShell();
   if (presShell) {
     presShell->HandleEventWithTarget(&event, this, nsnull, &status);
   }
 }
 
-//---------------------------------------------------------
 // Determine if the specified item in the listbox is selected.
 NS_IMETHODIMP
 nsListControlFrame::GetOptionSelected(PRInt32 aIndex, PRBool* aValue)
 {
   *aValue = IsContentSelectedByIndex(aIndex);
   return NS_OK;
 }
 
@@ -1690,17 +1635,16 @@ nsListControlFrame::OnSetSelectedIndex(P
 
   return NS_OK;
 }
 
 //----------------------------------------------------------------------
 // End nsISelectControlFrame
 //----------------------------------------------------------------------
 
-//---------------------------------------------------------
 nsresult
 nsListControlFrame::SetFormProperty(nsIAtom* aName,
                                 const nsAString& aValue)
 {
   if (nsGkAtoms::selected == aName) {
     return NS_ERROR_INVALID_ARG; // Selected is readonly according to spec.
   } else if (nsGkAtoms::selectedindex == aName) {
     // You shouldn't be calling me for this!!!
@@ -1708,17 +1652,16 @@ nsListControlFrame::SetFormProperty(nsIA
   }
 
   // We should be told about selectedIndex by the DOM element through
   // OnOptionSelected
 
   return NS_OK;
 }
 
-//---------------------------------------------------------
 nsresult 
 nsListControlFrame::GetFormProperty(nsIAtom* aName, nsAString& aValue) const
 {
   // Get the selected value of option from local cache (optimization vs. widget)
   if (nsGkAtoms::selected == aName) {
     nsAutoString val(aValue);
     PRInt32 error = 0;
     PRBool selected = PR_FALSE;
@@ -1732,62 +1675,58 @@ nsListControlFrame::GetFormProperty(nsIA
   } else if (nsGkAtoms::selectedindex == aName) {
     // You shouldn't be calling me for this!!!
     return NS_ERROR_INVALID_ARG;
   }
 
   return NS_OK;
 }
 
-//---------------------------------------------------------
 void
 nsListControlFrame::SyncViewWithFrame()
 {
     // Resync the view's position with the frame.
     // The problem is the dropdown's view is attached directly under
     // the root view. This means its view needs to have its coordinates calculated
     // as if it were in it's normal position in the view hierarchy.
   mComboboxFrame->AbsolutelyPositionDropDown();
 
   nsContainerFrame::PositionFrameView(this);
 }
 
-//---------------------------------------------------------
 void
 nsListControlFrame::AboutToDropDown()
 {
   if (mIsAllContentHere && mIsAllFramesHere && mHasBeenInitialized) {
     ScrollToIndex(GetSelectedIndex());
 #ifdef ACCESSIBILITY
     FireMenuItemActiveEvent(); // Inform assistive tech what got focus
 #endif
   }
   mItemSelectionStarted = PR_FALSE;
 }
 
-//---------------------------------------------------------
 // We are about to be rolledup from the outside (ComboboxFrame)
 void
 nsListControlFrame::AboutToRollup()
 {
   // We've been updating the combobox with the keyboard up until now, but not
   // with the mouse.  The problem is, even with mouse selection, we are
   // updating the <select>.  So if the mouse goes over an option just before
   // he leaves the box and clicks, that's what the <select> will show.
   //
   // To deal with this we say "whatever is in the combobox is canonical."
   // - IF the combobox is different from the current selected index, we
   //   reset the index.
 
   if (IsInDropDownMode()) {
-    ComboboxFinish(mComboboxFrame->GetIndexOfDisplayArea());
+    ComboboxFinish(mComboboxFrame->GetIndexOfDisplayArea()); // might destroy us
   }
 }
 
-//---------------------------------------------------------
 NS_IMETHODIMP
 nsListControlFrame::DidReflow(nsPresContext*           aPresContext,
                               const nsHTMLReflowState* aReflowState,
                               nsDidReflowStatus        aStatus)
 {
   nsresult rv;
   
   if (IsInDropDownMode()) 
@@ -1844,24 +1783,22 @@ nsListControlFrame::InvalidateInternal(c
 #ifdef DEBUG
 NS_IMETHODIMP
 nsListControlFrame::GetFrameName(nsAString& aResult) const
 {
   return MakeFrameName(NS_LITERAL_STRING("ListControl"), aResult);
 }
 #endif
 
-//---------------------------------------------------------
 nscoord
 nsListControlFrame::GetHeightOfARow()
 {
   return HeightOfARow();
 }
 
-//----------------------------------------------------------------------
 nsresult
 nsListControlFrame::IsOptionDisabled(PRInt32 anIndex, PRBool &aIsDisabled)
 {
   nsCOMPtr<nsISelectElement> sel(do_QueryInterface(mContent));
   if (sel) {
     sel->IsOptionDisabled(anIndex, &aIsDisabled);
     return NS_OK;
   }
@@ -2031,17 +1968,20 @@ nsListControlFrame::MouseUp(nsIDOMEvent*
       if (isDisabled) {
         aMouseEvent->PreventDefault();
         aMouseEvent->StopPropagation();
         CaptureMouseEvents(PR_FALSE);
         return NS_ERROR_FAILURE;
       }
 
       if (kNothingSelected != selectedIndex) {
+        nsWeakFrame weakFrame(this);
         ComboboxFinish(selectedIndex);
+        if (!weakFrame.IsAlive())
+          return NS_OK;
         FireOnChange();
       }
 
       mouseEvent->clickCount = 1;
     } else {
       // the click was out side of the select or its dropdown
       mouseEvent->clickCount = IgnoreMouseEventForSelection(aMouseEvent) ? 1 : 0;
     }
@@ -2049,70 +1989,47 @@ nsListControlFrame::MouseUp(nsIDOMEvent*
     CaptureMouseEvents(PR_FALSE);
     // Notify
     if (mChangesSinceDragStart) {
       // reset this so that future MouseUps without a prior MouseDown
       // won't fire onchange
       mChangesSinceDragStart = PR_FALSE;
       FireOnChange();
     }
-#if 0 // XXX - this is a partial fix for Bug 29990
-    if (mSelectedIndex != mStartExtendedIndex) {
-      mEndExtendedIndex = mSelectedIndex;
-    }
-#endif
   }
 
   return NS_OK;
 }
 
-/**
- * If the dropdown is showing and the mouse has moved below our
- * border-inner-edge, then set mItemSelectionStarted.
- */
 void
 nsListControlFrame::UpdateInListState(nsIDOMEvent* aEvent)
 {
   if (!mComboboxFrame || !mComboboxFrame->IsDroppedDown())
     return;
 
   nsPoint pt = nsLayoutUtils::GetDOMEventCoordinatesRelativeTo(aEvent, this);
   nsRect borderInnerEdge = GetScrollableView()->View()->GetBounds();
   if (pt.y >= borderInnerEdge.y && pt.y < borderInnerEdge.YMost()) {
     mItemSelectionStarted = PR_TRUE;
   }
 }
 
-/**
- * When the user clicks on the comboboxframe to show the dropdown
- * listbox, they then have to move the mouse into the list. We don't
- * want to process those mouse events as selection events (i.e., to
- * scroll list items into view). So we ignore the events until
- * the mouse moves below our border-inner-edge, when
- * mItemSelectionStarted is set.
- *
- * @param aPoint relative to this frame
- */
 PRBool nsListControlFrame::IgnoreMouseEventForSelection(nsIDOMEvent* aEvent)
 {
   if (!mComboboxFrame)
     return PR_FALSE;
 
   // Our DOM listener does get called when the dropdown is not
   // showing, because it listens to events on the SELECT element
   if (!mComboboxFrame->IsDroppedDown())
     return PR_TRUE;
 
   return !mItemSelectionStarted;
 }
 
-//----------------------------------------------------------------------
-// Fire a custom DOM event for the change, so that accessibility can
-// fire a native focus event for accessibility 
-// (Some 3rd party products need to track our focus)
 #ifdef ACCESSIBILITY
 void
 nsListControlFrame::FireMenuItemActiveEvent()
 {
   if (mFocused != this && !IsInDropDownMode()) {
     return;
   }
 
@@ -2132,22 +2049,16 @@ nsListControlFrame::FireMenuItemActiveEv
   if (!optionContent) {
     return;
   }
 
   FireDOMEvent(NS_LITERAL_STRING("DOMMenuItemActive"), optionContent);
 }
 #endif
 
-//----------------------------------------------------------------------
-// Sets the mSelectedIndex and mOldSelectedIndex from figuring out what 
-// item was selected using content
-// @param aPoint the event point, in listcontrolframe coordinates
-// @return NS_OK if it successfully found the selection
-//----------------------------------------------------------------------
 nsresult
 nsListControlFrame::GetIndexFromDOMEvent(nsIDOMEvent* aMouseEvent, 
                                          PRInt32&     aCurIndex)
 {
   if (IgnoreMouseEventForSelection(aMouseEvent))
     return NS_ERROR_FAILURE;
 
   nsIView* view = GetScrolledFrame()->GetView();
@@ -2206,17 +2117,16 @@ nsListControlFrame::GetIndexFromDOMEvent
       aCurIndex = numOptions - 1;
       return NS_OK;
     }
   }
 
   return NS_ERROR_FAILURE;
 }
 
-//----------------------------------------------------------------------
 nsresult
 nsListControlFrame::MouseDown(nsIDOMEvent* aMouseEvent)
 {
   NS_ASSERTION(aMouseEvent != nsnull, "aMouseEvent is null.");
 
   UpdateInListState(aMouseEvent);
 
   mButtonDown = PR_TRUE;
@@ -2257,17 +2167,22 @@ nsListControlFrame::MouseDown(nsIDOMEven
     if (mComboboxFrame) {
       if (!IgnoreMouseEventForSelection(aMouseEvent)) {
         return NS_OK;
       }
 
       if (!nsComboboxControlFrame::ToolkitHasNativePopup())
       {
         PRBool isDroppedDown = mComboboxFrame->IsDroppedDown();
+        nsIFrame* comboFrame;
+        CallQueryInterface(mComboboxFrame, &comboFrame);
+        nsWeakFrame weakFrame(comboFrame);
         mComboboxFrame->ShowDropDown(!isDroppedDown);
+        if (!weakFrame.IsAlive())
+          return NS_OK;
         if (isDroppedDown) {
           CaptureMouseEvents(PR_FALSE);
         }
       }
     }
   }
 
   return NS_OK;
@@ -2330,16 +2245,19 @@ nsListControlFrame::DragMove(nsIDOMEvent
       PRBool wasChanged = PerformSelection(selectedIndex,
                                            !isControl, isControl);
       mChangesSinceDragStart = mChangesSinceDragStart || wasChanged;
     }
   }
   return NS_OK;
 }
 
+//----------------------------------------------------------------------
+// Scroll helpers.
+//----------------------------------------------------------------------
 nsresult
 nsListControlFrame::ScrollToIndex(PRInt32 aIndex)
 {
   if (aIndex < 0) {
     // XXX shouldn't we just do nothing if we're asked to scroll to
     // kNothingSelected?
     return ScrollToFrame(nsnull);
   } else {
@@ -2347,17 +2265,16 @@ nsListControlFrame::ScrollToIndex(PRInt3
     if (content) {
       return ScrollToFrame(content);
     }
   }
 
   return NS_ERROR_FAILURE;
 }
 
-//----------------------------------------------------------------------
 nsresult
 nsListControlFrame::ScrollToFrame(nsIContent* aOptElement)
 {
   nsIScrollableView* scrollableView = GetScrollableView();
 
   if (scrollableView) {
     // if null is passed in we scroll to 0,0
     if (nsnull == aOptElement) {
@@ -2533,19 +2450,24 @@ nsListControlFrame::GetIncrementalString
 }
 
 void
 nsListControlFrame::DropDownToggleKey(nsIDOMEvent* aKeyEvent)
 {
   // Cocoa widgets do native popups, so don't try to show
   // dropdowns there.
   if (IsInDropDownMode() && !nsComboboxControlFrame::ToolkitHasNativePopup()) {
+    aKeyEvent->PreventDefault();
+    nsIFrame* comboFrame;
+    CallQueryInterface(mComboboxFrame, &comboFrame);
+    nsWeakFrame weakFrame(comboFrame);
     mComboboxFrame->ShowDropDown(!mComboboxFrame->IsDroppedDown());
+    if (!weakFrame.IsAlive())
+      return;
     mComboboxFrame->RedisplaySelectedText();
-    aKeyEvent->PreventDefault();
   }
 }
 
 nsresult
 nsListControlFrame::KeyPress(nsIDOMEvent* aKeyEvent)
 {
   NS_ASSERTION(aKeyEvent, "keyEvent is null.");
 
@@ -2623,28 +2545,36 @@ nsListControlFrame::KeyPress(nsIDOMEvent
       AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
                                 (PRInt32)numOptions,
                                 1, 1);
       } break;
 
     case nsIDOMKeyEvent::DOM_VK_RETURN: {
       if (mComboboxFrame != nsnull) {
         if (mComboboxFrame->IsDroppedDown()) {
+          nsWeakFrame weakFrame(this);
           ComboboxFinish(mEndSelectionIndex);
+          if (!weakFrame.IsAlive())
+            return NS_OK;
         }
         FireOnChange();
         return NS_OK;
       } else {
         newIndex = mEndSelectionIndex;
       }
       } break;
 
     case nsIDOMKeyEvent::DOM_VK_ESCAPE: {
+      nsWeakFrame weakFrame(this);
       AboutToRollup();
-      } break;
+      if (!weakFrame.IsAlive()) {
+        aKeyEvent->PreventDefault(); // since we won't reach the one below
+        return NS_OK;
+      }
+    } break;
 
     case nsIDOMKeyEvent::DOM_VK_PAGE_UP: {
       AdjustIndexForDisabledOpt(mEndSelectionIndex, newIndex,
                                 (PRInt32)numOptions,
                                 -(mNumDisplayRows-1), -1);
       } break;
 
     case nsIDOMKeyEvent::DOM_VK_PAGE_DOWN: {
--- a/layout/forms/nsListControlFrame.h
+++ b/layout/forms/nsListControlFrame.h
@@ -151,53 +151,91 @@ public:
 #endif
 
     // nsHTMLContainerFrame
   virtual PRIntn GetSkipSides() const;
 
     // nsIListControlFrame
   virtual void SetComboboxFrame(nsIFrame* aComboboxFrame);
   virtual PRInt32 GetSelectedIndex(); 
+
+  /**
+   * Gets the text of the currently selected item.
+   * If the there are zero items then an empty string is returned
+   * If there is nothing selected, then the 0th item's text is returned.
+   */
   virtual void GetOptionText(PRInt32 aIndex, nsAString & aStr);
+
   virtual void CaptureMouseEvents(PRBool aGrabMouseEvents);
   virtual nscoord GetHeightOfARow();
   virtual PRInt32 GetNumberOfOptions();  
   virtual void SyncViewWithFrame();
   virtual void AboutToDropDown();
+
+  /**
+   * @note This method might destroy |this|.
+   */
   virtual void AboutToRollup();
+
+  /**
+   * Dispatch a DOM onchange event synchroniously.
+   * @note This method might destroy |this|.
+   */
   virtual void FireOnChange();
+
+  /**
+   * Makes aIndex the selected option of a combobox list.
+   * @note This method might destroy |this|.
+   */
   virtual void ComboboxFinish(PRInt32 aIndex);
   virtual void OnContentReset();
 
   // nsISelectControlFrame
   NS_IMETHOD AddOption(nsPresContext* aPresContext, PRInt32 index);
   NS_IMETHOD RemoveOption(nsPresContext* aPresContext, PRInt32 index);
   NS_IMETHOD GetOptionSelected(PRInt32 aIndex, PRBool* aValue);
   NS_IMETHOD DoneAddingChildren(PRBool aIsDone);
+
+  /**
+   * Gets the content (an option) by index and then set it as
+   * being selected or not selected.
+   */
   NS_IMETHOD OnOptionSelected(nsPresContext* aPresContext,
                               PRInt32 aIndex,
                               PRBool aSelected);
   NS_IMETHOD OnSetSelectedIndex(PRInt32 aOldIndex, PRInt32 aNewIndex);
 
-  // mouse event listeners
+  // mouse event listeners (both might destroy |this|)
   nsresult MouseDown(nsIDOMEvent* aMouseEvent);
   nsresult MouseUp(nsIDOMEvent* aMouseEvent);
 
   // mouse motion listeners
   nsresult MouseMove(nsIDOMEvent* aMouseEvent);
   nsresult DragMove(nsIDOMEvent* aMouseEvent);
 
-  // key listeners
+  // key listener (might destroy |this|)
   nsresult KeyPress(nsIDOMEvent* aKeyEvent);
 
-  // Static Methods
+  /**
+   * Returns the options collection for aContent, if any.
+   */
   static already_AddRefed<nsIDOMHTMLOptionsCollection>
     GetOptions(nsIContent * aContent);
+
+  /**
+   * Returns the nsIDOMHTMLOptionElement for a given index 
+   * in the select's collection.
+   */
   static already_AddRefed<nsIDOMHTMLOptionElement>
     GetOption(nsIDOMHTMLOptionsCollection* aOptions, PRInt32 aIndex);
+
+  /**
+   * Returns the nsIContent object in the collection 
+   * for a given index.
+   */
   static already_AddRefed<nsIContent>
     GetOptionAsContent(nsIDOMHTMLOptionsCollection* aCollection,PRInt32 aIndex);
 
   static void ComboboxFocusSet();
 
   // Helper
   PRBool IsFocused() { return this == mFocused; }
 
@@ -227,43 +265,106 @@ public:
   }
 
   /**
    * Return whether the list is in dropdown mode.
    */
   PRBool IsInDropDownMode() const;
 
 #ifdef ACCESSIBILITY
+  /**
+   * Post a custom DOM event for the change, so that accessibility can
+   * fire a native focus event for accessibility 
+   * (Some 3rd party products need to track our focus)
+   */
   void FireMenuItemActiveEvent(); // Inform assistive tech what got focused
 #endif
 
 protected:
-  // Returns PR_FALSE if calling it destroyed |this|.
+  /**
+   * Updates the selected text in a combobox and then calls FireOnChange().
+   * Returns PR_FALSE if calling it destroyed |this|.
+   */
   PRBool     UpdateSelection();
+
+  /**
+   * Returns whether the nsIDOMHTMLSelectElement supports 
+   * multiple selection.
+   */
   PRBool     GetMultiple(nsIDOMHTMLSelectElement* aSelect = nsnull) const;
+
+  /**
+   * Toggles (show/hide) the combobox dropdown menu.
+   * @note This method might destroy |this|.
+   */
   void       DropDownToggleKey(nsIDOMEvent* aKeyEvent);
+
   nsresult   IsOptionDisabled(PRInt32 anIndex, PRBool &aIsDisabled);
   nsresult   ScrollToFrame(nsIContent * aOptElement);
   nsresult   ScrollToIndex(PRInt32 anIndex);
+
+  /**
+   * When the user clicks on the comboboxframe to show the dropdown
+   * listbox, they then have to move the mouse into the list. We don't
+   * want to process those mouse events as selection events (i.e., to
+   * scroll list items into view). So we ignore the events until
+   * the mouse moves below our border-inner-edge, when
+   * mItemSelectionStarted is set.
+   *
+   * @param aPoint relative to this frame
+   */
   PRBool     IgnoreMouseEventForSelection(nsIDOMEvent* aEvent);
+
+  /**
+   * If the dropdown is showing and the mouse has moved below our
+   * border-inner-edge, then set mItemSelectionStarted.
+   */
   void       UpdateInListState(nsIDOMEvent* aEvent);
   void       AdjustIndexForDisabledOpt(PRInt32 aStartIndex, PRInt32 &anNewIndex,
                                        PRInt32 aNumOptions, PRInt32 aDoAdjustInc, PRInt32 aDoAdjustIncNext);
+
+  /**
+   * Resets the select back to it's original default values;
+   * those values as determined by the original HTML
+   */
   virtual void ResetList(PRBool aAllowScrolling);
 
   nsListControlFrame(nsIPresShell* aShell, nsIDocument* aDocument, nsStyleContext* aContext);
   virtual ~nsListControlFrame();
 
   // Utility methods
   nsresult GetSizeAttribute(PRInt32 *aSize);
   nsIContent* GetOptionFromContent(nsIContent *aContent);
+
+  /**
+   * Sets the mSelectedIndex and mOldSelectedIndex from figuring out what 
+   * item was selected using content
+   * @param aPoint the event point, in listcontrolframe coordinates
+   * @return NS_OK if it successfully found the selection
+   */
   nsresult GetIndexFromDOMEvent(nsIDOMEvent* aMouseEvent, PRInt32& aCurIndex);
+
+  /**
+   * For a given index it returns the nsIContent object 
+   * from the select.
+   */
   already_AddRefed<nsIContent> GetOptionContent(PRInt32 aIndex) const;
+
+  /** 
+   * For a given piece of content, it determines whether the 
+   * content (an option) is selected or not.
+   * @return PR_TRUE if it is, PR_FALSE if it is NOT.
+   */
   PRBool   IsContentSelected(nsIContent* aContent) const;
+
+  /**
+   * For a given index is return whether the content is selected.
+   */
   PRBool   IsContentSelectedByIndex(PRInt32 aIndex) const;
+
   PRBool   IsOptionElement(nsIContent* aContent);
   PRBool   CheckIfAllFramesHere();
   PRInt32  GetIndexFromContent(nsIContent *aContent);
   PRBool   IsLeftButton(nsIDOMEvent* aMouseEvent);
 
   // aNumOptions is the number of options we have; if we have none,
   // we'll just guess at a row height based on our own style.
   nscoord  CalcFallbackRowHeight(PRInt32 aNumOptions);