[webkit-reviews] review denied: [Bug 131637] Clean up unnecessary methods in the BackForwardClient interface : [Attachment 232426] rebased patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brian Burg <burg at cs.washington.edu>'s
request for review:
Bug 131637: Clean up unnecessary methods in the BackForwardClient interface
https://bugs.webkit.org/show_bug.cgi?id=131637

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232426&action=review


Excellent effort, but unfortunately this breaks the WebBackForwardList APIs on
Mac and Windows, so needs more work.

> Source/WebCore/page/FrameView.cpp:2287
> +    if (!view.frame().page())
> +	   return false;

This isn’t mentioned in the change log. Part of another patch perhaps? (I
suggest using a local variable for the page.)

> Source/WebKit/mac/History/WebBackForwardList.mm:95
> +    _private =
reinterpret_cast<WebBackForwardListPrivate*>(backForwardList);

This won’t work. This WebBackForwardList has a pointer to the underlying
BackForwardList, but the underlying list can be deleted underneath it. If a
client calls a function like -[WebBackForwardList addItem:] after the
BackForwardList is destroyed, it could result in the dereference of a deleted
pointer; use after free is not good.

This shows us a fundamental problem with changing the BackForwardList to no
longer be reference counted. The public WebBackForwardList API makes a contract
that it is reference counted. We could possibly get away with having a
WebBackForwardList hold some kind of weak pointer to the underlying
BackForwardList, but we can’t simply hold a pointer without any lifetime
guarantee at all.

There are all kinds of alternate strategies we could try. For example a
WebBackForwardList could be changed to hold a reference to the main frame of a
page, and then we could go from the main frame to the page and then in turn to
the back/forward list. Or we could literally put a WeakPtrFactory into the
BackForwardList class, although that would not be great for platforms that
don’t have a public WebBackForwardList API.

> Source/WebKit/win/WebBackForwardList.h:112
> +    WebCore::BackForwardList* m_backForwardList;

Same problem here as on the Mac: The back forward list can be deleted, leaving
the WebBackForwardList object in a state where function calls on it will use
after free.

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:141
>  
> +

Extra blank line here.

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:146
> +    HashSet<uint64_t>::iterator end = m_associatedItemIDs.end();
> +    for (HashSet<uint64_t>::iterator i = m_associatedItemIDs.begin(); i !=
end; ++i)
> +	   WebCore::pageCache()->remove(itemForID(*i));

Should use a modern for loop here.

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:148
> +    m_associatedItemIDs.clear();

This isn’t necessary. The set is about to be destroyed.