Summary: | REGRESSION (r167775): Safari crashes in ViewSnapshotStore::pruneSnapshots after loading 20 pages | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||
Component: | WebKit2 | Assignee: | 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
Tim Horton
2014-04-25 14:14:38 PDT
Created attachment 230199 [details]
patch
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) This didn't fix the problem 100%. 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. 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 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? |