Bug 132204 – REGRESSION (r167775): Safari crashes in ViewSnapshotStore::pruneSnapshots after loading 20 pages

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

[Tim Horton]

Typo (and also a separate mistake not causing a crash but causing the code to run more than it should).

<rdar://problem/16729123>

[Tim Horton]

Created attachment 230199 [details]
patch

[Anders Carlsson]

Comment on attachment 230199 [details]
patch

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

> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:79
> +        const auto& snapshot = m_snapshotMap.find(snapshotUUID);

I don't think you should call this snapshot since it's an iterator. How about 

auto snapshotIt = m_snapshotMap.find(snapshotUUID)

[Tim Horton]

https://trac.webkit.org/changeset/167822

[Tim Horton]

This didn't fix the problem 100%.

[Tim Horton]

Created attachment 230245 [details]
followup

Comment on attachment 230245 [details]
followup

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

> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:113
> +    if (snapshotIter == m_snapshotMap.end()) {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }

This is better written as

ASSERT(snapshotIter != m_snapshotMap.end());
if (snapshowIter == m_snapshotMap.end())
    return;

Since the assertion failure message will say what is the condition that was false.

[Tim Horton]

Sam thinks that the early return is sloppy and that we should just keep crashing if it's wrong so that we fix it; http://trac.webkit.org/changeset/167849.

[Darin Adler]

Comment on attachment 230245 [details]
followup

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

> Source/WebKit2/UIProcess/mac/ViewSnapshotStore.mm:109
> +    const auto& snapshotIter = m_snapshotMap.find(oldestSnapshotUUID);

Why const auto& instead of auto?