[webkit-reviews] review granted: [Bug 87417] (Unrepro) Crashes saving session state in WebBackForwardList : [Attachment 143897] Patch v4 - Hopefully last iteration - remove some unnecessary if (m_page) checks.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 87417: (Unrepro) Crashes saving session state in WebBackForwardList
https://bugs.webkit.org/show_bug.cgi?id=87417

Attachment 143897: Patch v4 - Hopefully last iteration - remove some
unnecessary if (m_page) checks.
https://bugs.webkit.org/attachment.cgi?id=143897&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.