Bug 26087

Summary:

[Chromium] Removing element in JS crashes Chrome tab if it fired the change event

Product:

WebKit

Reporter:

Victor Wang <victorw>

Component:

Platform

Assignee:

Victor Wang <victorw>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, victorw

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

All

URL:

http://www.schrierc.org/chrome-reload-crash.html

Attachments:

Description	Flags
Porposed fix for bug 26087	none
Proposed fix for bug 26087	dglazkov: review-
Move test to chromium directory	dglazkov: review-
Move test to chromium directory	none
Move test to chromium directory	dglazkov: review+

Victor Wang

Reported 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

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Victor Wang

Comment 1 2009-05-29 15:00:51 PDT

Created attachment 30791 [details] Porposed fix for bug 26087

Victor Wang

Comment 2 2009-06-03 10:07:44 PDT

Created attachment 30910 [details] Proposed fix for bug 26087

Dimitri Glazkov (Google)

Comment 3 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.

Victor Wang

Comment 4 2009-06-03 10:57:46 PDT

Created attachment 30913 [details] Move test to chromium directory

Dimitri Glazkov (Google)

Comment 5 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.

Victor Wang

Comment 6 2009-06-03 11:16:15 PDT

Created attachment 30915 [details] Move test to chromium directory

Victor Wang

Comment 7 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();

Victor Wang

Comment 8 2009-06-03 11:22:33 PDT

Created attachment 30916 [details] Move test to chromium directory

Victor Wang

Comment 9 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.

Dimitri Glazkov (Google)

Comment 10 2009-06-08 09:24:53 PDT

Landed as http://trac.webkit.org/changeset/44500.

Note You need to log in before you can comment on or make changes to this bug.