Bug 67074 – REGRESSION (r93913): Failures in fast/replaced/frame-removed-during-resize.html and scrollbars/scrollable-iframe-remove-crash.html

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Levin]

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]

[Martin Robinson]

This also occurs on WebKitGTK+.

[Dmitry Titov]

Created attachment 105555 [details]
Patch

[WebKit Review Bot]

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.

[Darin Adler]

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”.

[Dmitry Titov]

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.

[David Levin]

*** Bug 67197 has been marked as a duplicate of this bug. ***

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/94088