Bug 133370 – Crash loading skydrive.com (assertion under RemoteLayerTreeDisplayRefreshMonitor)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

The set of monitors is being mutated while iterating the set of monitors...

<rdar://problem/17061361>

[Tim Horton]

Created attachment 232236 [details]
patch

[Anders Carlsson]

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]

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

[Tim Horton]

Reopening to address post-landing review comments.

[Tim Horton]

Created attachment 232272 [details]
patch

[Geoffrey Garen]

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 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 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]

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