Bug 26087 - [Chromium] Removing element in JS crashes Chrome tab if it fired the change event
Summary: [Chromium] Removing element in JS crashes Chrome tab if it fired the change e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Victor Wang
URL: http://www.schrierc.org/chrome-reload...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-29 14:49 PDT by Victor Wang
Modified: 2009-06-08 09:24 PDT (History)
2 users (show)

See Also:


Attachments
Porposed fix for bug 26087 (2.59 KB, patch)
2009-05-29 15:00 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Proposed fix for bug 26087 (4.16 KB, patch)
2009-06-03 10:07 PDT, Victor Wang
dglazkov: review-
Details | Formatted Diff | Diff
Move test to chromium directory (6.64 KB, patch)
2009-06-03 10:57 PDT, Victor Wang
dglazkov: review-
Details | Formatted Diff | Diff
Move test to chromium directory (6.04 KB, patch)
2009-06-03 11:16 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Move test to chromium directory (6.04 KB, patch)
2009-06-03 11:22 PDT, Victor Wang
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 2009-05-29 14:49:31 PDT
This applies to Chrome only.
If a popup list is abandoned (press a key to jump to an item and then use tab
or mouse to get away from the select box), the current code in PopupMenuChromium fires a change event in updateFromElemt(). The JS that listens to this event may
destroy the object and cause the rest of popup list code crashes.

What steps will reproduce the problem?

1. Open URL using Chrome: http://www.schrierc.org/chrome-reload-crash.html
2. Click the select
3. Press 's' on your keyboard
4. Click on the document but not the select itself
Comment 1 Victor Wang 2009-05-29 15:00:51 PDT
Created attachment 30791 [details]
Porposed fix for bug 26087
Comment 2 Victor Wang 2009-06-03 10:07:44 PDT
Created attachment 30910 [details]
Proposed fix for bug 26087
Comment 3 Dimitri Glazkov (Google) 2009-06-03 10:34:22 PDT
Comment on attachment 30910 [details]
Proposed fix for bug 26087

> Index: WebCore/manual-tests/onchange-reload-popup.html
> ===================================================================
> --- WebCore/manual-tests/onchange-reload-popup.html	(revision 0)

This should probably go to WebCore/manual-tests/chromium/onchange-reload-popup.html

Looks good otherwise.
Comment 4 Victor Wang 2009-06-03 10:57:46 PDT
Created attachment 30913 [details]
Move test to chromium directory
Comment 5 Dimitri Glazkov (Google) 2009-06-03 11:04:13 PDT
Comment on attachment 30913 [details]
Move test to chromium directory

> +        (WebCore::PopupListBox::updateFromElement):
> +
> +<<<<<<< .mine

Be careful :) -- you left merge markers in the ChangeLog.

Also, when uploading a new patch to Bugzilla, make sure to mark the old patch as obsolete and mark the new patch for review. It's tedious, but that's the way.
Comment 6 Victor Wang 2009-06-03 11:16:15 PDT
Created attachment 30915 [details]
Move test to chromium directory
Comment 7 Victor Wang 2009-06-03 11:17:44 PDT
Comment on attachment 30915 [details]
Move test to chromium directory

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 44387)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,37 @@
>+2009-06-03  victorw  <victorw@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        https://bugs.webkit.org/show_bug.cgi?id=26087
>+        Bug 26087: Removing element in JS crashes Chrome tab if it fired the change event
>+        
>+        Fix tab crash caused by destroying the popup list that fired the change event on abandon.
>+        
>+        If a popup list is abandoned (press a key to jump to an item
>+        and then use tab or mouse to get away from the select box),
>+        the current code fires a change event in PopupListBox::updateFromElemt().
>+        The JS that listens to this event may destroy the object and cause the
>+        rest of popup list code crashes.
>+        
>+        The updateFromElement() is called before abandon() and this causes
>+        the selected index to be discarded after updateFromElement(). From
>+        the code comments, this appears to be the reason why valueChanged is
>+        called in updateFromElement.
>+        
>+        Fix the issue by removing the valueChanged call in updateFromElement,
>+        saving the selected index that we should accept on abandon and pass
>+        it to the valueChange in abandon().
>+        
>+        A manual test has been added.
>+        
>+        * manual-tests/chromium: Added.
>+        * manual-tests/chromium/onchange-reload-popup.html: Added.
>+        * platform/chromium/PopupMenuChromium.cpp:
>+        (WebCore::PopupListBox::PopupListBox):
>+        (WebCore::PopupListBox::handleKeyEvent):
>+        (WebCore::PopupListBox::abandon):
>+        (WebCore::PopupListBox::updateFromElement):
>+
> 2009-06-03  Kevin Watters  <kevinwatters@gmail.com>
> 
>         Reviewed by Kevin Ollivier.
>Index: WebCore/manual-tests/chromium/onchange-reload-popup.html
>===================================================================
>--- WebCore/manual-tests/chromium/onchange-reload-popup.html	(revision 0)
>+++ WebCore/manual-tests/chromium/onchange-reload-popup.html	(revision 0)
>@@ -0,0 +1,44 @@
>+<html>
>+<head>
>+    <script type="text/javascript"> 
>+
>+    function addEvent(obj, evType, fn) {
>+        if (obj.addEventListener){
>+          obj.addEventListener(evType, fn, false);
>+          return true;
>+        } else if (obj.attachEvent){
>+          var r = obj.attachEvent("on"+evType, fn);
>+          return r;
>+        } else {
>+          return false;
>+        }
>+    }
>+
>+    function reloadSelect() {
>+        var container = document.getElementById('container');
>+        container.innerHTML = '<select id="menu"> \
>+          <option value="abcd">abcd</option>\
>+          <option value="defg">efgh</option>\
>+        </select>';
>+
>+        var menu = document.getElementById('menu');
>+        addEvent(menu, 'change', reloadSelect);
>+    }
>+
>+    </script>
>+</head>
>+<body>
>+    <p>Do the following and see if Chromium crashes.</p>
>+    <ul>
>+      <li>Click the select</li> 
>+      <li>Press 'e' on your keyboard</li> 
>+      <li>Click on the document but not the select itself.</li>
>+    </ul>
>+
>+    <div id="container"/>
>+    <script>
>+        reloadSelect()
>+    </script>
>+    </div> 
>+</body>
>+</html>
>
>Property changes on: WebCore/manual-tests/chromium/onchange-reload-popup.html
>___________________________________________________________________
>Added: svn:executable
>   + *
>
>Index: WebCore/platform/chromium/PopupMenuChromium.cpp
>===================================================================
>--- WebCore/platform/chromium/PopupMenuChromium.cpp	(revision 44269)
>+++ WebCore/platform/chromium/PopupMenuChromium.cpp	(working copy)
>@@ -153,7 +153,7 @@ private:
>         : m_settings(settings)
>         , m_originalIndex(0)
>         , m_selectedIndex(0)
>-        , m_willAcceptOnAbandon(false)
>+        , m_acceptedIndexOnAbandon(-1)
>         , m_visibleRows(0)
>         , m_popupClient(client)
>         , m_repeatingChar(0)
>@@ -222,11 +222,11 @@ private:
>     // enter yet however.
>     int m_selectedIndex;
> 
>-    // True if we should accept the selectedIndex as chosen, even if the popup
>-    // is "abandoned".  This is used for keyboard navigation, where we want the
>+    // If >= 0, this is the index we should accept if the popup is "abandoned".
>+    // This is used for keyboard navigation, where we want the
>     // selection to change immediately, and is only used if the settings
>     // acceptOnAbandon field is true.
>-    bool m_willAcceptOnAbandon;
>+    int m_acceptedIndexOnAbandon;
> 
>     // This is the number of rows visible in the popup. The maximum number visible at a time is
>     // defined as being kMaxVisibleRows. For a scrolled popup, this can be thought of as the
>@@ -648,7 +648,7 @@ bool PopupListBox::handleKeyEvent(const 
>         // IE).  We change the original index so we revert to that when the
>         // popup is closed.
>         if (m_settings.acceptOnAbandon)
>-            m_willAcceptOnAbandon = true;
>+            m_acceptedIndexOnAbandon = m_selectedIndex;
> 
>         setOriginalIndex(m_selectedIndex);
>         if (m_settings.setTextOnIndexChange)
>@@ -838,8 +838,10 @@ void PopupListBox::abandon()
> 
>     m_popupClient->hidePopup();
> 
>-    if (m_willAcceptOnAbandon)
>-        m_popupClient->valueChanged(m_selectedIndex);
>+    if (m_acceptedIndexOnAbandon >= 0) {
>+        m_popupClient->valueChanged(m_acceptedIndexOnAbandon);
>+        m_acceptedIndexOnAbandon = -1;
>+    }
> }
> 
> int PopupListBox::pointToRowIndex(const IntPoint& point)
>@@ -1018,14 +1020,6 @@ void PopupListBox::adjustSelectedIndex(i
> 
> void PopupListBox::updateFromElement()
> {
>-    // It happens when pressing a key to jump to an item, then use tab or
>-    // mouse to get away from the select box. In that case, updateFromElement
>-    // is called before abandon, which causes discarding of the select result.    
>-    if (m_willAcceptOnAbandon) {
>-        m_popupClient->valueChanged(m_selectedIndex);
>-        m_willAcceptOnAbandon = false;
>-    }
>-
>     clear();
> 
>     int size = m_popupClient->listSize();
Comment 8 Victor Wang 2009-06-03 11:22:33 PDT
Created attachment 30916 [details]
Move test to chromium directory
Comment 9 Victor Wang 2009-06-03 11:26:23 PDT
(In reply to comment #5)
> (From update of attachment 30913 [details] [review])
> > +        (WebCore::PopupListBox::updateFromElement):
> > +
> > +<<<<<<< .mine
> 
> Be careful :) -- you left merge markers in the ChangeLog.
> 
> Also, when uploading a new patch to Bugzilla, make sure to mark the old patch
> as obsolete and mark the new patch for review. It's tedious, but that's the
> way.
> 

Got merge issue and some format issue in ChangeLog. All fixed by latest patch.
Comment 10 Dimitri Glazkov (Google) 2009-06-08 09:24:53 PDT
Landed as http://trac.webkit.org/changeset/44500.