Fix PopupMenuChromium to hide itself before calling valueChanged. This bette...

third_party/WebKit/WebCore/platform/chromium/PopupMenuChromium.cpp

Index: third_party/WebKit/WebCore/platform/chromium/PopupMenuChromium.cpp
===================================================================
--- third_party/WebKit/WebCore/platform/chromium/PopupMenuChromium.cpp	(revision 9010)
+++ third_party/WebKit/WebCore/platform/chromium/PopupMenuChromium.cpp	(working copy)
@@ -73,6 +73,11 @@
 // box ("combobox" on Windows).
 class PopupListBox : public FramelessScrollView, public RefCounted<PopupListBox> {
 public:
+    static PassRefPtr<PopupListBox> create(PopupMenuClient* client)
+    {
+        return adoptRef(new PopupListBox(client));
+    }
+
     // FramelessScrollView
     virtual void paint(GraphicsContext*, const IntRect&);
     virtual bool handleMouseDownEvent(const PlatformMouseEvent&);
@@ -323,13 +328,9 @@
 }
 
 PopupContainer::PopupContainer(PopupMenuClient* client, bool focusOnShow)
-    : m_listBox(new PopupListBox(client)),
-      m_focusOnShow(focusOnShow)
+    : m_listBox(PopupListBox::create(client))
+    , m_focusOnShow(focusOnShow)
 {
-    // FrameViews are created with a refcount of 1 so it needs releasing after we
-    // assign it to a RefPtr.
-    m_listBox->deref();
-
     setScrollbarModes(ScrollbarAlwaysOff, ScrollbarAlwaysOff);
 }
 
@@ -361,11 +362,11 @@
         chromeClient->popupOpened(this, widgetRect, m_focusOnShow);
     }
 
-    // Must get called after we have a client and containingWindow.
-    addChild(m_listBox.get());
+    if (!m_listBox->parent())
+        addChild(m_listBox.get());
 
-    // Enable scrollbars after the listbox is inserted into the hierarchy, so
-    // it has a proper WidgetClient.
+    // Enable scrollbars after the listbox is inserted into the hierarchy,
+    // so it has a proper WidgetClient.
     m_listBox->setVerticalScrollbarMode(ScrollbarAuto);
 
     m_listBox->scrollToRevealSelection();
@@ -375,12 +376,6 @@
 
 void PopupContainer::hidePopup()
 {
-    invalidate();
-
-    m_listBox->disconnectClient();
-    removeChild(m_listBox.get());
-    m_listBox = 0;
-    
     if (client())
         client()->popupClosed(this);
 }
@@ -856,12 +851,10 @@
 
     m_selectedIndex = m_originalIndex;
 
+    m_popupClient->hidePopup();
+
     if (m_willAcceptOnAbandon)
         m_popupClient->valueChanged(m_selectedIndex);
-
-    // valueChanged may have torn down the popup!
-    if (m_popupClient)
-        m_popupClient->hidePopup();
 }
 
 int PopupListBox::pointToRowIndex(const IntPoint& point)
@@ -893,12 +886,11 @@
     if (isSelectableItem(index)) {
         RefPtr<PopupListBox> keepAlive(this);
 
-        // Tell the <select> PopupMenuClient what index was selected, and hide ourself.
-        m_popupClient->valueChanged(index);
+        // Hide ourselves first since valueChanged may have numerous side-effects.
+        m_popupClient->hidePopup();
 
-        // valueChanged may have torn down the popup!
-        if (m_popupClient)
-            m_popupClient->hidePopup();
+        // Tell the <select> PopupMenuClient what index was selected.
+        m_popupClient->valueChanged(index);
     }
 }
 
@@ -1157,16 +1149,15 @@
 
 void PopupMenu::show(const IntRect& r, FrameView* v, int index) 
 {
-    p.popup = PopupContainer::create(client(), true);
+    if (!p.popup)
+        p.popup = PopupContainer::create(client(), true);
     p.popup->show(r, v, index);
 }
 
 void PopupMenu::hide()
 {
-    if (p.popup) {
+    if (p.popup)
         p.popup->hidePopup();
-        p.popup = 0;
-    }
 }
 
 void PopupMenu::updateFromElement()