Bug 87417 – (Unrepro) Crashes saving session state in WebBackForwardList

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

[Brady Eidson]

(Unrepro) Crashes saving session state in WebBackForwardList

We've seen crashes here:
 1 com.apple.WebKit2  WebKit::WebURL::create(WTF::String const&) 
 2 com.apple.WebKit2  WebKit::WebBackForwardList::createCFDictionaryRepresentation(bool (*)(OpaqueWKPage const*, OpaqueWKString const*, void const*, void*), void*) const
 3 com.apple.WebKit2  WebKit::WebPageProxy::sessionStateData(bool (*)(OpaqueWKPage const*, OpaqueWKString const*, void const*, void*), void*) const
 4 com.apple.WebKit2  WKPageCopySessionState

Unfortunately we don't know how to reproduce it.

Digging in to crash logs and disassembly it is clear that one of the entries in WebBackForwardList::m_entries is null.

When looking at the current code it's clear there's a few safeguards we can take to avoiding writing bogus sessions out to disk and to avoid adding a bogus new item to the back/forward list.

In radar as <rdar://problem/10090764>

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 143876 [details]
Patch v1

[WebKit Review Bot]

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.

[Brady Eidson]

Created attachment 143880 [details]
Patch v2 - Now with more style.

[Brady Eidson]

Created attachment 143894 [details]
Patch v3 - Remove an unneeded ASSERT and add one addition currentIndex safeguard when writing to disk.

[Brady Eidson]

Created attachment 143897 [details]
Patch v4 - Hopefully last iteration - remove some unnecessary if (m_page) checks.

[Darin Adler]

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.

[Brady Eidson]

(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!

[Brady Eidson]

http://trac.webkit.org/changeset/118441

[Brady Eidson]

Required a followup http://trac.webkit.org/changeset/118443 to fix the build with super strict warnings.