[webkit-reviews] review granted: [Bug 34722] [Chromium] Confirm index is valid in BackForwardListClientImpl::itemAtIndex() before returning non-null : [Attachment 48364] patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Nate Chapin
<japhet at chromium.org>'s request for review:
Bug 34722: [Chromium] Confirm index is valid in
BackForwardListClientImpl::itemAtIndex() before returning non-null
https://bugs.webkit.org/show_bug.cgi?id=34722

Attachment 48364: patch
https://bugs.webkit.org/attachment.cgi?id=48364&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.