Bug 67074 - REGRESSION (r93913): Failures in fast/replaced/frame-removed-during-resize.html and scrollbars/scrollable-iframe-remove-crash.html
Summary: REGRESSION (r93913): Failures in fast/replaced/frame-removed-during-resize.ht...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows 2000
: P2 Normal
Assignee: Dmitry Titov
URL:
Keywords:
: 67197 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-08-26 17:45 PDT by David Levin
Modified: 2011-08-30 10:35 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.51 KB, patch)
2011-08-29 18:10 PDT, Dmitry Titov
dimich: commit-queue-
Details | Formatted Diff | Diff
Updated patch (3.28 KB, patch)
2011-08-29 18:45 PDT, Dmitry Titov
darin: review+
dimich: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-08-26 17:45:55 PDT
So far I see this on Chromium Linux but it is in generic code so it seems like other platforms may be affected (so I'm cc'ing ap). I'm doing a OS X WebKit build right now to check it out.

dimich said that he would look into fixing this.

ASSERTION FAILED: !m_queuedEvents.isEmpty()
third_party/WebKit/Source/WebCore/dom/EventQueue.cpp(121) : void WebCore::EventQueue::pendingEventTimerFired()
[25891:25891:4881599612:ERROR:process_util_posix.cc(134)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x8c8f8e]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x885b2d]
	0x7f18575d4af0
	WebCore::EventQueue::pendingEventTimerFired() [0xe2970a]
	WebCore::EventQueueTimer::fired() [0xe29af8]
	WebCore::ThreadTimers::sharedTimerFiredInternal() [0xfd2fb4]
	WebCore::ThreadTimers::sharedTimerFired() [0xfd2eeb]
	webkit_glue::WebKitClientImpl::DoTimeout() [0x19c70c8]
Comment 1 Martin Robinson 2011-08-29 08:40:53 PDT
This also occurs on WebKitGTK+.
Comment 2 Dmitry Titov 2011-08-29 18:10:11 PDT
Created attachment 105555 [details]
Patch
Comment 3 WebKit Review Bot 2011-08-29 18:11:52 PDT
Attachment 105555 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 9

Updating OpenSource
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 150.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Darin Adler 2011-08-29 18:25:12 PDT
Comment on attachment 105555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105555&action=review

> Source/WebCore/dom/EventQueue.cpp:118
> +// The accumulated and future events should be discarded. 
> +// This method is called when Document is prepared for destruction.
>  void EventQueue::cancelQueuedEvents()

This changes the semantics of this function. It’s OK because the one caller wants a behavior different from what the function name implies, but would not OK if there was more than one caller. The function’s name should change along with its change in semantics. It now has a meaning something more like “close” or “shut down” or “stop”.
Comment 5 Dmitry Titov 2011-08-29 18:45:07 PDT
Created attachment 105561 [details]
Updated patch

Thanks! Renamed the method.

Here is more info on what exactly caused the crashes: The EventQueue contains a loop that processes all the accumulated events. Now with the r93913 landed, the Frame may be destroyed upon exiting from event dispatch. This may immediately queue more events  (Frame::setSelection() in this case, clearing the frame's selection state) - and the new events would be processed on the next loop iteration, while the timer which is started when new event is enqued would be not reset. Next time the timer fires we get an empty queue (because the event was actually processed before) and ASSERT fires.

Instead of removing ASSERT and say it's ok when events enque other events (this apparently started to happen after r93913), I think we can stop accepting new events after EventQueue was explicitly 'stopped', which is a good thing to do anyways. That covers the cases of failing tests and still keeps ASSERT in place so we can find out what other events can cause new events to be added.
Comment 6 David Levin 2011-08-30 06:54:15 PDT
*** Bug 67197 has been marked as a duplicate of this bug. ***
Comment 7 Dmitry Titov 2011-08-30 10:35:43 PDT
Landed: http://trac.webkit.org/changeset/94088