Bug 34722 - [Chromium] Confirm index is valid in BackForwardListClientImpl::itemAtIndex() before returning non-null
Summary: [Chromium] Confirm index is valid in BackForwardListClientImpl::itemAtIndex()...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-08 13:17 PST by Nate Chapin
Modified: 2010-02-10 09:02 PST (History)
1 user (show)

See Also:


Attachments
patch (1.17 KB, patch)
2010-02-08 13:52 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
layout test (1.61 KB, patch)
2010-02-08 16:25 PST, Nate Chapin
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-02-08 13:17:54 PST
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.
Comment 1 Nate Chapin 2010-02-08 13:52:44 PST
Created attachment 48364 [details]
patch
Comment 2 Darin Fisher (:fishd, Google) 2010-02-08 14:00:45 PST
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.
Comment 3 Nate Chapin 2010-02-08 14:47:50 PST
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.
Comment 4 Eric Seidel (no email) 2010-02-08 15:12:15 PST
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.
Comment 5 Nate Chapin 2010-02-08 16:25:10 PST
Created attachment 48375 [details]
layout test

Sorry for the delay in getting this test out.
Comment 6 Chris Evans 2010-02-08 17:07:52 PST
Do we know what revision broke this?
Comment 7 Adam Barth 2010-02-09 12:31:02 PST
Comment on attachment 48375 [details]
layout test

I support all passing tests.
Comment 8 Nate Chapin 2010-02-10 09:02:15 PST
http://trac.webkit.org/changeset/54600