Bug 34722 – [Chromium] Confirm index is valid in BackForwardListClientImpl::itemAtIndex() before returning non-null

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

Chromium bug: http://crbug.com/33929

If a history.forward() call occurs, for example, at the start of a page load, we will end up cancelling the original load because itemAtIndex() is returning non-null even though the forward list is empty.

[Nate Chapin]

Created attachment 48364 [details]
patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 48364 [details]
patch

Please add a link to this bug in the ChangeLog entry.


> Index: WebKit/chromium/src/BackForwardListClientImpl.cpp
> ===================================================================
> --- WebKit/chromium/src/BackForwardListClientImpl.cpp	(revision 54505)
> +++ WebKit/chromium/src/BackForwardListClientImpl.cpp	(working copy)
> @@ -90,7 +90,7 @@
>  
>  HistoryItem* BackForwardListClientImpl::itemAtIndex(int index)
>  {
> -    if (!m_webView->client())
> +    if (!m_webView->client() || index > forwardListCount() || index < (backListCount() * -1))

It might read slightly better as:

  if (!m_webView->client() || index > forwardListCount() || -index > backListCount())

Either way, R=me

Please file a bug about adding a layout test per our discussion.  For those reading at
home, Nate's initial attempts at creating a test didn't pan out, and this bug is a
release blocker for Chrome, so we are splitting the task into two bugs.  He'll work on
completing the layout test next.

[Nate Chapin]

Comment on attachment 48364 [details]
patch

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

If it's alright, I'll leave this bug open for the layout test that I will be submitting ASAP.

[Eric Seidel (no email)]

Comment on attachment 48364 [details]
patch

Cleared Darin Fisher's review+ from obsolete attachment 48364 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Nate Chapin]

Created attachment 48375 [details]
layout test

Sorry for the delay in getting this test out.

[Chris Evans]

Do we know what revision broke this?

[Adam Barth]

Comment on attachment 48375 [details]
layout test

I support all passing tests.

[Nate Chapin]

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