Bug 132204

Summary: REGRESSION (r167775): Safari crashes in ViewSnapshotStore::pruneSnapshots after loading 20 pages
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, dino, mitz, mrowe, rniwa, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
andersca: review+
followup mitz: review+

Description Tim Horton 2014-04-25 14:14:38 PDT
Typo (and also a separate mistake not causing a crash but causing the code to run more than it should).

<rdar://problem/16729123>
Comment 1 Tim Horton 2014-04-25 14:21:27 PDT
Created attachment 230199 [details]
patch
Comment 2 Anders Carlsson 2014-04-25 14:24:08 PDT
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)
Comment 3 Tim Horton 2014-04-25 14:29:22 PDT
https://trac.webkit.org/changeset/167822
Comment 4 Tim Horton 2014-04-26 15:28:38 PDT
This didn't fix the problem 100%.
Comment 5 Tim Horton 2014-04-26 15:33:19 PDT
Created attachment 230245 [details]
followup
Comment 6 mitz 2014-04-26 19:37:21 PDT
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.
Comment 7 Tim Horton 2014-04-26 19:45:17 PDT
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.
Comment 8 Darin Adler 2014-04-27 11:16:46 PDT
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?