WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
26087
[Chromium] Removing element in JS crashes Chrome tab if it fired the change event
https://bugs.webkit.org/show_bug.cgi?id=26087
Summary
[Chromium] Removing element in JS crashes Chrome tab if it fired the change e...
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.
Top of Page
Format For Printing
XML
Clone This Bug