Bug 131637 – Clean up unnecessary methods in the BackForwardClient interface

• Component: History
• Status: NEW

[Brian Burg]

Or, remove the FIXME from BackForwardClient if this is no longer the plan. I could not find many uses from WebCore, and WK/WK2 uses could just use methods specific to the implementors of BackForwardClient.

[Anders Carlsson]

No, we should get rid of them.

[Darin Adler]

Yes, this is really a Chromium remnant.

[Darin Adler]

BackForwardClient abstracts what BackForwardList does for WebKit1, but is instead proxied for WebKit2. Any WebKit1-only functions can simply be on BackForwardList and need not be in BackForwardClient.

Besides backItem, currentItem, and forwardItem, that also includes the current, setCurrent, and clearAllPageCaches functions. All six of them should be removed, and made non-virtual in BackForwardList.

Another thing that’s wrong with BackForwardClient is that it should not be reference counted. It has no shared ownership need that I can see. So BackForwardController::m_client should be a unique_ptr rather than a RefPtr. And the BackForwardController::client function should return a reference, not a pointer. And we should consider getting rid of it completely; it’s only there so WebKit1 code can recover the BackForwardList pointer, and there must be some better way to do that without a typecast involved.

[Brian Burg]

(In reply to comment #3)
> Another thing that’s wrong with BackForwardClient is that it should not be reference counted. It has no shared ownership need that I can see. So BackForwardController::m_client should be a unique_ptr rather than a RefPtr. And the BackForwardController::client function should return a reference, not a pointer. And we should consider getting rid of it completely; it’s only there so WebKit1 code can recover the BackForwardList pointer, and there must be some better way to do that without a typecast involved.


If BackForwardClient is owned, who owns it? In Cocoa WK2, it's passed in from WebPage. If none is provided, the controller makes its own.

[Brian Burg]

Created attachment 229919 [details]
the patch

[WebKit Commit Bot]

Attachment 229919 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:2015:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 229919 [details]
the patch

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

Thanks for tackling this!

> Source/WebCore/history/BackForwardClient.h:31
> -#include <wtf/Forward.h>
> -#include <wtf/RefCounted.h>
> +#include <wtf/PassRefPtr.h>

Why include PassRefPtr.h instead of Forward.h?

> Source/WebCore/history/BackForwardController.h:42
> +    BackForwardController(Page&, BackForwardClient*);

This should take a std::unique_ptr, not a raw pointer, since this object needs to take ownership.

> Source/WebCore/history/BackForwardController.h:71
> +    BackForwardClient* m_client;

This needs to be a std::unique_ptr, otherwise the client will never be destroyed.

> Source/WebCore/history/BackForwardList.h:44
> +    explicit BackForwardList(Page*);

Can this take a reference instead of a pointer?

> Source/WebCore/history/BackForwardList.h:84
>      Page* m_page;

Can this be a reference instead of a pointer?

> Source/WebCore/page/Page.cpp:1605
> +    , backForwardClient(nullptr)

We should not make this change.

> Source/WebCore/page/Page.h:140
> +        BackForwardClient* backForwardClient;

We should make this a std::unique_ptr.

> Source/WebKit/mac/History/WebBackForwardList.mm:116
> +    return [self initWithBackForwardList:new BackForwardList(0)];

This is no good. It leaks the BackForwardList. Someone needs to own it. We’ll probably have to leave it RefCounted if we can’t figure out a single-ownership model.

> Source/WebKit/mac/History/WebBackForwardListInternal.h:39
> +- (id)initWithBackForwardList:(WebCore::BackForwardList*)backForwardList;

Not sure this makes sense; what’s the lifetime relationship between the WebBackForwardList and the WebCore::BackForwardList? I think we might have to go back to making it RefCounted.

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:39
> +    explicit WebBackForwardListProxy(WebPage*);

Can this take a reference instead of a pointer?

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:64
>      WebPage* m_page;

Can this be a reference instead of a pointer?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:322
> +    pageClients.backForwardClient = new WebBackForwardListProxy(this);

Where is the code to delete this? For the drag client, for example, the function that deletes it is WebDragClient::dragControllerDestroyed. But it looks like this just leaks. I suggest using std::make_unique here.

[Brian Burg]

Created attachment 230034 [details]
retry w/ unique_ptr

[WebKit Commit Bot]

Attachment 230034 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:2016:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Burg]

(In reply to comment #7)
> (From update of attachment 229919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229919&action=review
> 

Thanks for the feedback Darin. Despite some skepticism, I was able to redo the patch using unique_ptr semantics. It seems to check out on Mac WK1/2, but it will definitely break GTK, EFL, Win bindings of the history list. Once we are happy with the mac approach I'll look at tweaking other ports.

[Darin Adler]

Comment on attachment 230034 [details]
retry w/ unique_ptr

/mnt/eflews/webkit/WebKit/Source/WebKit/efl/ewk/ewk_history.cpp: In function 'Eina_Bool ewk_history_clear(Ewk_History*)':
/mnt/eflews/webkit/WebKit/Source/WebKit/efl/ewk/ewk_history.cpp:97:33: error: 'class WebCore::BackForwardList' has no member named 'page'
     WebCore::Page* page = core->page();
                                 ^
/mnt/eflews/webkit/WebKit/Source/WebKit/efl/ewk/ewk_history.cpp: In function 'Ewk_History* ewk_history_new(WebCore::BackForwardList*)':
/mnt/eflews/webkit/WebKit/Source/WebKit/efl/ewk/ewk_history.cpp:375:11: error: 'class WebCore::BackForwardList' has no member named 'ref'
     core->ref();
           ^
/mnt/eflews/webkit/WebKit/Source/WebKit/efl/ewk/ewk_history.cpp: In function 'void ewk_history_free(Ewk_History*)':
/mnt/eflews/webkit/WebKit/Source/WebKit/efl/ewk/ewk_history.cpp:391:20: error: 'class WebCore::BackForwardList' has no member named 'deref'
     history->core->deref();

     1>..\..\win\WebView.cpp(3074): error C2440: 'static_cast' : cannot convert from 'WebCore::BackForwardClient' to 'WebCore::BackForwardList *'
                 No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
     1>..\..\win\WebView.cpp(3907): error C2819: type 'WebCore::BackForwardClient' does not have an overloaded member 'operator ->'
                 c:\cygwin\home\buildbot\webkit\webkitbuild\release\include\webcore\BackForwardClient.h(37) : see declaration of 'WebCore::BackForwardClient'
                 did you intend to use '.' instead?
     1>..\..\win\WebView.cpp(3907): error C2039: 'backItem' : is not a member of 'WebCore::BackForwardClient'
                 c:\cygwin\home\buildbot\webkit\webkitbuild\release\include\webcore\BackForwardClient.h(37) : see declaration of 'WebCore::BackForwardClient'
     1>..\..\win\WebView.cpp(3922): error C2819: type 'WebCore::BackForwardClient' does not have an overloaded member 'operator ->'
                 c:\cygwin\home\buildbot\webkit\webkitbuild\release\include\webcore\BackForwardClient.h(37) : see declaration of 'WebCore::BackForwardClient'
                 did you intend to use '.' instead?
     1>..\..\win\WebView.cpp(3922): error C2039: 'forwardItem' : is not a member of 'WebCore::BackForwardClient'
                 c:\cygwin\home\buildbot\webkit\webkitbuild\release\include\webcore\BackForwardClient.h(37) : see declaration of 'WebCore::BackForwardClient'
     1>..\..\win\WebView.cpp(5403): error C2440: 'initializing' : cannot convert from 'WebCore::BackForwardClient' to 'WebCore::BackForwardClient *'
                 No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
     1>..\..\win\WebView.cpp(5409): error C2440: 'initializing' : cannot convert from 'WebCore::BackForwardClient' to 'WebCore::BackForwardClient *'
                 No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
     1>..\..\win\WebView.cpp(5410): error C2039: 'currentItem' : is not a member of 'WebCore::BackForwardClient'
                 c:\cygwin\home\buildbot\webkit\webkitbuild\release\include\webcore\BackForwardClient.h(37) : see declaration of 'WebCore::BackForwardClient'

[Darin Adler]

Comment on attachment 230034 [details]
retry w/ unique_ptr

Approach looks fine for Mac

[Brian Burg]

Created attachment 230338 [details]
try to fix win, efl

[WebKit Commit Bot]

Attachment 230338 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:2020:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 230338 [details]
try to fix win, efl

1>..\..\win\WebView.cpp(3074): error C2440: 'static_cast' : cannot convert from 'WebCore::BackForwardClient' to 'WebCore::BackForwardList *'
                 No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

[Brian Burg]

Created attachment 230432 [details]
actually fix windows

[WebKit Commit Bot]

Attachment 230432 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:2020:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 230432 [details]
actually fix windows

Attachment 230432 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6455336802189312

New failing tests:
compositing/geometry/video-fixed-scrolling.html

[Build Bot]

Created attachment 230449 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Andreas Kling]

Comment on attachment 230432 [details]
actually fix windows

r=me

[Brian Burg]

Committed r168041: <http://trac.webkit.org/changeset/168041>

[Simon Fraser (smfr)]

Broke an API test:
http://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/4321

[Myles C. Maxfield]

Reopening due to broken API test

[Brian Burg]

Tracking a fix in https://bugs.webkit.org/show_bug.cgi?id=132405

It seems to fix everything locally on mac-mountainlion; building on mac-mavericks now.

[Myles C. Maxfield]

Looks like a WK1 test is crashing in addition. http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r168041%20(5637)/http/tests/navigation/no-referrer-reset-crash-log.txt

[Benjamin Poulain]

(In reply to comment #25)
> Looks like a WK1 test is crashing in addition. http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r168041%20(5637)/http/tests/navigation/no-referrer-reset-crash-log.txt

Hum, I get a ton of crashes on iOS with this patch.

[Alexey Proskuryakov]

I don't understand why we don't roll out everything and land clean with issues resolved. The test has been broken for an unacceptably long period of time.

[Brian Burg]

(In reply to comment #27)
> I don't understand why we don't roll out everything and land clean with issues resolved. The test has been broken for an unacceptably long period of time.

Recapping IRC: we decided to do a manual rollout since there are too many broken things to tell whether the fix in 132045 actually works.

(If the EWS results had any relationship to the build dashboard, a lot of this trouble would be avoided IMO.)

[Alexey Proskuryakov]

Rolled out in <http://trac.webkit.org/r168085>.

[Alexey Proskuryakov]

When fixing the patch, it may be a good idea to run some tests with GuardMalloc. I had random crashes running tests locally (on a revision before the rollout), and GuardMalloc made these deterministic, pointing to BackForwardList code.

run-webkit-tests -g

[Brian Burg]

Created attachment 230601 [details]
combined patch

[WebKit Commit Bot]

Attachment 230601 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:2024:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Burg]

(In reply to comment #31)
> Created an attachment (id=230601) [details]
> combined patch

I haven't been able to reproduce any more test failures or crashes using the combined patches. So, I'm ready to retry landing.

However, I would appreciate if someone could test the patch on iOS. benjaminp reported some GuardMalloc runs pointed to problems with BackForwardList, but I don't remember if that included the follow-up fix to BackForwardClient::close().

[Brian Burg]

Created attachment 232426 [details]
rebased patch

[WebKit Commit Bot]

Attachment 232426 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:2003:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 232426 [details]
rebased patch

Attachment 232426 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6276292534075392

New failing tests:
fast/loader/image-in-page-cache.html

[Build Bot]

Created attachment 232433 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 232426 [details]
rebased patch

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.

[Darin Adler]

(In reply to comment #33)
> benjaminp reported some GuardMalloc runs pointed to problems with BackForwardList

These are possibly due to the dangling pointers to the back/forward list from the WebBackForwardList. The code I referred to as Mac is both Mac and iOS.

[Brian Burg]

Comment on attachment 232426 [details]
rebased patch

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

Thanks for looking at this. I'll give it another try on the weekend.

>> 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.

(It's a little surprising we don't have a test case that tries to keep the WebBackForwardList around after the page is deallocated.)

I'm in favor of storing a reference to the MainFrame, since the same approach could also work for Windows and EFL's API (which has the same lifetime issues).

[Brian Burg]

Created attachment 232695 [details]
wip, stuck on DRT crashes