WebBackForwardList::backForwardListState(filter) can set the currentIndex to a really large number

[Ada Chan]

If the currentIndex is 0 and the first item gets filtered out, we'll end up decrementing currentIndex (which is bad as it's unsigned).

See the code here:

    for (size_t i = 0; i < m_entries.size(); ++i) {
        auto& entry = *m_entries[i];

        if (filter && !filter(entry)) {
            if (backForwardListState.currentIndex && i <= backForwardListState.currentIndex.value())
                --backForwardListState.currentIndex.value();

            continue;
        }

        backForwardListState.items.append(entry.itemState());
    }

Seems like we should check whether the currentIndex > 0 before decrementing it. And if the list items end up being all filtered out, then the currentIndex should be set to the "uninitialized" value.

[Ada Chan]

Created attachment 252516 [details]
Patch

[Darin Adler]

Comment on attachment 252516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252516&action=review

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:401
> +            if (backForwardListState.currentIndex && i <= backForwardListState.currentIndex.value() && backForwardListState.currentIndex.value())
>                  --backForwardListState.currentIndex.value();

Common subexpression is getting out of hand here. Here’s one way to deal with that

    auto& index = backForwardListState.currentIndex;
    if (index && i <= index.value() && index.value())
        --index.value();

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:410
> +        backForwardListState.currentIndex = Optional<uint32_t>();

There are two other ways to write this that are both more terse:

    backForwardListState.currentIndex = Nullopt;
    backForwardListState.currentIndex = { };

I suggest using one of those.

[Ada Chan]

Thanks Darin.  I'll incorporate your feedback and post an updated patch (this time with a test).

[Ada Chan]

Created attachment 252566 [details]
Updated patch

[Ada Chan]

I ended up removing the Util::run(&didFinishLoad) after restoring the page with sessionStateWithAllItemsRemoved because nothing really gets loaded in that case.  I'll add a comment about that.

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/183933