Bug 131637 - Clean up unnecessary methods in the BackForwardClient interface
Summary: Clean up unnecessary methods in the BackForwardClient interface
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 132405
Blocks: 132027
  Show dependency treegraph
 
Reported: 2014-04-14 15:05 PDT by Brian Burg
Modified: 2015-08-03 19:59 PDT (History)
16 users (show)

See Also:


Attachments
the patch (23.86 KB, patch)
2014-04-22 16:18 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
retry w/ unique_ptr (37.50 KB, patch)
2014-04-23 19:16 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
try to fix win, efl (45.56 KB, patch)
2014-04-28 17:24 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
actually fix windows (49.82 KB, patch)
2014-04-29 16:06 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (906.89 KB, application/zip)
2014-04-29 19:04 PDT, Build Bot
no flags Details
combined patch (53.74 KB, patch)
2014-05-01 12:13 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
rebased patch (54.21 KB, patch)
2014-06-03 09:00 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (498.92 KB, application/zip)
2014-06-03 10:33 PDT, Build Bot
no flags Details
wip, stuck on DRT crashes (58.66 KB, patch)
2014-06-08 20:20 PDT, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-04-14 15:05:34 PDT
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.
Comment 1 Anders Carlsson 2014-04-17 11:52:17 PDT
No, we should get rid of them.
Comment 2 Darin Adler 2014-04-19 13:43:31 PDT
Yes, this is really a Chromium remnant.
Comment 3 Darin Adler 2014-04-19 13:57:13 PDT
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.
Comment 4 Brian Burg 2014-04-22 15:09:33 PDT
(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.
Comment 5 Brian Burg 2014-04-22 16:18:05 PDT
Created attachment 229919 [details]
the patch
Comment 6 WebKit Commit Bot 2014-04-22 16:20:13 PDT
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.
Comment 7 Darin Adler 2014-04-22 18:31:55 PDT
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.
Comment 8 Brian Burg 2014-04-23 19:16:23 PDT
Created attachment 230034 [details]
retry w/ unique_ptr
Comment 9 WebKit Commit Bot 2014-04-23 19:17:29 PDT
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.
Comment 10 Brian Burg 2014-04-23 19:19:31 PDT
(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.
Comment 11 Darin Adler 2014-04-24 17:32:07 PDT
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'
Comment 12 Darin Adler 2014-04-24 17:34:32 PDT
Comment on attachment 230034 [details]
retry w/ unique_ptr

Approach looks fine for Mac
Comment 13 Brian Burg 2014-04-28 17:24:16 PDT
Created attachment 230338 [details]
try to fix win, efl
Comment 14 WebKit Commit Bot 2014-04-28 17:25:43 PDT
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.
Comment 15 Darin Adler 2014-04-29 09:06:17 PDT
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
Comment 16 Brian Burg 2014-04-29 16:06:38 PDT
Created attachment 230432 [details]
actually fix windows
Comment 17 WebKit Commit Bot 2014-04-29 16:08:03 PDT
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.
Comment 18 Build Bot 2014-04-29 19:04:07 PDT
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
Comment 19 Build Bot 2014-04-29 19:04:12 PDT
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
Comment 20 Andreas Kling 2014-04-30 13:35:58 PDT
Comment on attachment 230432 [details]
actually fix windows

r=me
Comment 21 Brian Burg 2014-04-30 13:42:45 PDT
Committed r168041: <http://trac.webkit.org/changeset/168041>
Comment 23 Myles C. Maxfield 2014-04-30 20:20:41 PDT
Reopening due to broken API test
Comment 24 Brian Burg 2014-04-30 20:25:00 PDT
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.
Comment 26 Benjamin Poulain 2014-04-30 21:40:21 PDT
(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.
Comment 27 Alexey Proskuryakov 2014-04-30 22:19:15 PDT
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.
Comment 28 Brian Burg 2014-04-30 22:49:37 PDT
(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.)
Comment 29 Alexey Proskuryakov 2014-04-30 22:50:14 PDT
Rolled out in <http://trac.webkit.org/r168085>.
Comment 30 Alexey Proskuryakov 2014-05-01 11:36:49 PDT
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
Comment 31 Brian Burg 2014-05-01 12:13:40 PDT
Created attachment 230601 [details]
combined patch
Comment 32 WebKit Commit Bot 2014-05-01 17:10:26 PDT
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.
Comment 33 Brian Burg 2014-05-04 09:55:23 PDT
(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().
Comment 34 Brian Burg 2014-06-03 09:00:23 PDT
Created attachment 232426 [details]
rebased patch
Comment 35 WebKit Commit Bot 2014-06-03 09:03:02 PDT
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.
Comment 36 Build Bot 2014-06-03 10:33:26 PDT
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
Comment 37 Build Bot 2014-06-03 10:33:32 PDT
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
Comment 38 Darin Adler 2014-06-05 09:39:08 PDT
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.
Comment 39 Darin Adler 2014-06-05 09:40:15 PDT
(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.
Comment 40 Brian Burg 2014-06-05 10:13:47 PDT
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).
Comment 41 Brian Burg 2014-06-08 20:20:49 PDT
Created attachment 232695 [details]
wip, stuck on DRT crashes