| Summary: | (Unrepro) Crashes saving session state in WebBackForwardList | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> |
| Component: | WebKit2 | Assignee: | Brady Eidson <beidson> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | darin, webkit.review.bot |
| Priority: | P2 | Keywords: | InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | Mac | ||
| OS: | All | ||
| Attachments: | |||
|
Description
Brady Eidson
2012-05-24 13:00:05 PDT
Also filed https://bugs.webkit.org/show_bug.cgi?id=87418 to track the greater job of revamping the whole class as a separate task. Created attachment 143876 [details]
Patch v1
Attachment 143876 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/WebBackForwardList.cpp:64: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 143880 [details]
Patch v2 - Now with more style.
Created attachment 143894 [details]
Patch v3 - Remove an unneeded ASSERT and add one addition currentIndex safeguard when writing to disk.
Created attachment 143897 [details]
Patch v4 - Hopefully last iteration - remove some unnecessary if (m_page) checks.
Comment on attachment 143897 [details] Patch v4 - Hopefully last iteration - remove some unnecessary if (m_page) checks. View in context: https://bugs.webkit.org/attachment.cgi?id=143897&action=review I’m OK with this as is, but it might be worth putting together a patch that is even more conservative and doesn’t change things that don’t need to change. We were going to follow up with a patch that makes a bigger impact. > Source/WebKit2/UIProcess/WebBackForwardList.cpp:102 > + m_entries.insert(m_current, newItem); Where’s the guarantee that m_current is <= m_entries.size()? Wasn’t that one of the things we were worried about? I thought we were going to put a check for that close to here to make us safer against logic mistakes. > Source/WebKit2/UIProcess/WebBackForwardList.cpp:-150 > - // Do range checks without doing math on index to avoid overflow. I like that comment. Why remove it? > Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:129 > + uint32_t currentIndex = currentCFIndex == -1 ? NoCurrentItemIndex : currentCFIndex; This can chop off currentCFIndex, which might be a large number that won’t fit in a uint32_t. Maybe better to do this after checking currentCFIndex against the size? > Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:187 > - // Perform a sanity check: in case we're out of range, we reset. > - if (m_current != NoCurrentItemIndex && m_current >= newEntries.size()) > - m_current = NoCurrentItemIndex; > + > + ASSERT(m_current == NoCurrentItemIndex || m_current < m_entries.size()); I agree with this change, but perhaps it can be left to later. Not sure it helps prevent the bug. (In reply to comment #7) > (From update of attachment 143897 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143897&action=review > > I’m OK with this as is, but it might be worth putting together a patch that is even more conservative and doesn’t change things that don’t need to change. We were going to follow up with a patch that makes a bigger impact. With the mysterious nature of the bug and the intense sleuthing we've done in to it, I feel like I can see potential value in almost everything included here from the original proposed patch. Except for the sanity check that your last comment is about. > > Source/WebKit2/UIProcess/WebBackForwardList.cpp:102 > > + m_entries.insert(m_current, newItem); > > Where’s the guarantee that m_current is <= m_entries.size()? Wasn’t that one of the things we were worried about? I thought we were going to put a check for that close to here to make us safer against logic mistakes. ACK - Accidentally got rid of it while pruning down the original patch you'd seen. I've made it: // m_current never be pointing more than 1 past the end of the entries Vector. // If it is, something has gone wrong and we should not try to insert the new item. ASSERT(m_current <= m_entries.size()); if (m_current <= m_entries.size()) m_entries.insert(m_current, newItem); > > Source/WebKit2/UIProcess/WebBackForwardList.cpp:-150 > > - // Do range checks without doing math on index to avoid overflow. > > I like that comment. Why remove it? Restored. > > Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:129 > > + uint32_t currentIndex = currentCFIndex == -1 ? NoCurrentItemIndex : currentCFIndex; > > This can chop off currentCFIndex, which might be a large number that won’t fit in a uint32_t. Maybe better to do this after checking currentCFIndex against the size? Done. > > Source/WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:187 > > - // Perform a sanity check: in case we're out of range, we reset. > > - if (m_current != NoCurrentItemIndex && m_current >= newEntries.size()) > > - m_current = NoCurrentItemIndex; > > + > > + ASSERT(m_current == NoCurrentItemIndex || m_current < m_entries.size()); > > I agree with this change, but perhaps it can be left to later. Not sure it helps prevent the bug. The sanity check really was worthless, but yes it can wait until later. Thanks for the review! Required a followup http://trac.webkit.org/changeset/118443 to fix the build with super strict warnings. |