Bug 133370

Summary:

Crash loading skydrive.com (assertion under RemoteLayerTreeDisplayRefreshMonitor)

Product:

WebKit

Reporter:

Tim Horton <thorton>

Component:

WebKit2

Assignee:

Tim Horton <thorton>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, simon.fraser

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	andersca: review+
patch	darin: review+

Tim Horton

Reported 2014-05-29 00:12:41 PDT

The set of monitors is being mutated while iterating the set of monitors... <rdar://problem/17061361>

Attachments
patch (1.80 KB, patch) 2014-05-29 00:15 PDT, Tim Horton	andersca: review+	Details Formatted Diff Diff
patch (4.46 KB, patch) 2014-05-29 17:03 PDT, Tim Horton	darin: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Tim Horton

Comment 1 2014-05-29 00:15:08 PDT

Created attachment 232236 [details] patch

Anders Carlsson

Comment 2 2014-05-29 06:44:59 PDT

Comment on attachment 232236 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232236&action=review > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:333 > + HashSet<RemoteLayerTreeDisplayRefreshMonitor*> monitors = m_displayRefreshMonitors; I think you should use a Vector and copy the hash set to it instead, that might be faster!

Tim Horton

Comment 3 2014-05-29 11:20:05 PDT

http://trac.webkit.org/changeset/169456

Tim Horton

Comment 4 2014-05-29 16:38:47 PDT

Reopening to address post-landing review comments.

Tim Horton

Comment 5 2014-05-29 17:03:55 PDT

Created attachment 232272 [details] patch

Geoffrey Garen

Comment 6 2014-05-29 19:30:30 PDT

Comment on attachment 232272 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232272&action=review > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:343 > + HashSet<RemoteLayerTreeDisplayRefreshMonitor*> monitorsToNotify = m_displayRefreshMonitors; > + m_displayRefreshMonitorsToNotify = &monitorsToNotify; > + while (!monitorsToNotify.isEmpty()) > + monitorsToNotify.takeAny()->didUpdateLayers(); > + m_displayRefreshMonitorsToNotify = nullptr; > } Because takeAny() uses the begin() iterator, it has to iterate a chunk of the hash table each time to find the first non-null bucket. So, switching to takeAny() may not be a performance improvement.

Darin Adler

Comment 7 2014-05-29 19:59:17 PDT

Comment on attachment 232272 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232272&action=review >> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:343 >> } > > Because takeAny() uses the begin() iterator, it has to iterate a chunk of the hash table each time to find the first non-null bucket. So, switching to takeAny() may not be a performance improvement. The change is for correctness, not performance improvement.

Darin Adler

Comment 8 2014-05-29 20:00:54 PDT

Comment on attachment 232272 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=232272&action=review > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:339 > + m_displayRefreshMonitorsToNotify = &monitorsToNotify; If would be good to ASSERT(!m_displayRefreshMonitorsToNotify) here.

Tim Horton

Comment 9 2014-05-30 08:44:50 PDT

http://trac.webkit.org/changeset/169486

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