Bug 168676 – Refactor WebViewImpl creation in preparation for supporting multiple WebsiteDataStores

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Refactor WebViewImpl creation in preparation for supporting multiple WebsiteDataStores

Currently no behavior change in practice.

[Brady Eidson]

Created attachment 302311 [details]
Patch

[Brady Eidson]

Created attachment 302316 [details]
Patch

[Alex Christensen]

Comment on attachment 302316 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/WebViewImpl.mm:1239
> +    return std::unique_ptr<WebViewImpl>(new WebViewImpl(view, WTFMove(pageClient), *webPage, processPool));

std::make_unique

[Sam Weinig]

(In reply to comment #0)
> Refactor WebViewImpl creation in preparation for supporting multiple
> WebsiteDataStores

What do you mean by supporting multiple WebsiteDataStores?

[Sam Weinig]

Comment on attachment 302316 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:554
> +        [NSException raise:NSInternalInconsistencyException format:@"[WKWebView _initializeWithConfiguration:] failed to create a new WebProcess"];

Given that _initializeWithConfiguration is an implementation detail, it seems like a bad thing to put in an exception that can be consumed by third parties.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:908
> +    _data->_impl = WebViewImpl::maybeCreate(self, nullptr, processPool, WTFMove(configuration));

Under what scenarios does this fail?

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #0)
> > Refactor WebViewImpl creation in preparation for supporting multiple
> > WebsiteDataStores
> 
> What do you mean by supporting multiple WebsiteDataStores?

Currently there's no concept of more than one non-default, persistent WKWebsiteDataStore in the Cocoa API. There's about to be!

(In reply to comment #5)
> Comment on attachment 302316 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302316&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:554
> > +        [NSException raise:NSInternalInconsistencyException format:@"[WKWebView _initializeWithConfiguration:] failed to create a new WebProcess"];
> 
> Given that _initializeWithConfiguration is an implementation detail, it
> seems like a bad thing to put in an exception that can be consumed by third
> parties.

Good point.

> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:908
> > +    _data->_impl = WebViewImpl::maybeCreate(self, nullptr, processPool, WTFMove(configuration));
> 
> Under what scenarios does this fail?

If we attempt to make a WK(Web)View with a non-default data store but we're at the process cap.

e.g. - In practice, it won't.

Note: This seemingly artificial limitation is being imposed because too much of the data store types are currently WebProcess-global instead of being per-WK(Web)View.

It can be unwound in the future after more refactoring.

[Brady Eidson]

Created attachment 302341 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 302341 [details]
Patch

Clearing flags on attachment: 302341

Committed r212786: <http://trac.webkit.org/changeset/212786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Comment on attachment 302341 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:554
> +        [NSException raise:NSInternalInconsistencyException format:@"WKWebView initialization failed. Unable to create a new WebProcess."];

Is it normal to raise an exception on resource exhaustion? Maybe this is OK because the programmer set the resource limits?

> Source/WebKit2/UIProcess/WebProcessPool.cpp:760
> +    // If a non-default WebsiteDataStore was passed in and we couldn't make a new web process for it,
> +    // we should not return an existing process.

Is it impossible that there is an existing Web process with the same non-default data store?

[Brady Eidson]

(In reply to comment #10)
> Comment on attachment 302341 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302341&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:554
> > +        [NSException raise:NSInternalInconsistencyException format:@"WKWebView initialization failed. Unable to create a new WebProcess."];
> 
> Is it normal to raise an exception on resource exhaustion? Maybe this is OK
> because the programmer set the resource limits?

That's the goal - programmer set the limits - and will be tested.

> 
> > Source/WebKit2/UIProcess/WebProcessPool.cpp:760
> > +    // If a non-default WebsiteDataStore was passed in and we couldn't make a new web process for it,
> > +    // we should not return an existing process.
> 
> Is it impossible that there is an existing Web process with the same
> non-default data store?

It is possible, and that can certainly be an enhancement.

[Carlos Garcia Campos]

Comment on attachment 302341 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessPool.cpp:762
> +    // If a non-default WebsiteDataStore was passed in and we couldn't make a new web process for it,
> +    // we should not return an existing process.
> +    if (websiteDataStore && websiteDataStore != &API::WebsiteDataStore::defaultDataStore()->websiteDataStore())
> +        return nullptr;

This has broken GTK+ port, because we never use the API::WebsiteDataStore::defaultDataStore(). We always use WebsiteDataStore::create() passing a configuration for persistent data stores and WebsiteDataStore::createNonPersistentDataStore() for ephemeral.

[Carlos Garcia Campos]

Comment on attachment 302341 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessPool.cpp:542
> +WebProcessProxy& WebProcessPool::createNewWebProcess(WebsiteDataStore* websiteDataStore)

Why are we passing the data store here? It's not actually used.

[Carlos Garcia Campos]

Comment on attachment 302341 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessPool.cpp:800
> +        process = maybeCreateNewWebProcessRespectingProcessCountLimit(&pageConfiguration->websiteDataStore()->websiteDataStore());
>  
>      return process->createWebPage(pageClient, WTFMove(pageConfiguration));

This is a problem too, maybeCreateNewWebProcessRespectingProcessCountLimit cna return nullptr now, but process is used without any null check.

[Carlos Garcia Campos]

I was thinking about a possible workaround for GTK+, but our bots are exiting early, so I'm going to roll it out while we think of a better fix for all ports.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 168710

[Ryan Haddad]

It seems that this change may have also caused API test failures, so we should look into that too before landing again:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/3786

[Sam Weinig]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #0)
> > > Refactor WebViewImpl creation in preparation for supporting multiple
> > > WebsiteDataStores
> > 
> > What do you mean by supporting multiple WebsiteDataStores?
> 
> Currently there's no concept of more than one non-default, persistent
> WKWebsiteDataStore in the Cocoa API. There's about to be!
> 
> (In reply to comment #5)
> > Comment on attachment 302316 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=302316&action=review
> > 
> > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:554
> > > +        [NSException raise:NSInternalInconsistencyException format:@"[WKWebView _initializeWithConfiguration:] failed to create a new WebProcess"];
> > 
> > Given that _initializeWithConfiguration is an implementation detail, it
> > seems like a bad thing to put in an exception that can be consumed by third
> > parties.
> 
> Good point.
> 
> > 
> > > Source/WebKit2/UIProcess/API/mac/WKView.mm:908
> > > +    _data->_impl = WebViewImpl::maybeCreate(self, nullptr, processPool, WTFMove(configuration));
> > 
> > Under what scenarios does this fail?
> 
> If we attempt to make a WK(Web)View with a non-default data store but we're
> at the process cap.
> 
> e.g. - In practice, it won't.
> 
> Note: This seemingly artificial limitation is being imposed because too much
> of the data store types are currently WebProcess-global instead of being
> per-WK(Web)View.
> 
> It can be unwound in the future after more refactoring.

It seems like bumping past the limit would probably be better than throwing, if this will be fixed in the future.

[Brady Eidson]

(In reply to comment #12)
> Comment on attachment 302341 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302341&action=review
> 
> > Source/WebKit2/UIProcess/WebProcessPool.cpp:762
> > +    // If a non-default WebsiteDataStore was passed in and we couldn't make a new web process for it,
> > +    // we should not return an existing process.
> > +    if (websiteDataStore && websiteDataStore != &API::WebsiteDataStore::defaultDataStore()->websiteDataStore())
> > +        return nullptr;
> 
> This has broken GTK+ port, because we never use the
> API::WebsiteDataStore::defaultDataStore(). We always use
> WebsiteDataStore::create() passing a configuration for persistent data
> stores and WebsiteDataStore::createNonPersistentDataStore() for ephemeral.

Ugh. Bummer.

[Brady Eidson]

(In reply to comment #13)
> Comment on attachment 302341 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302341&action=review
> 
> > Source/WebKit2/UIProcess/WebProcessPool.cpp:542
> > +WebProcessProxy& WebProcessPool::createNewWebProcess(WebsiteDataStore* websiteDataStore)
> 
> Why are we passing the data store here? It's not actually used.

Part of a multi-stage refactor - That's going to be used in the patch for https://bugs.webkit.org/show_bug.cgi?id=168696

[Brady Eidson]

(In reply to comment #18)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (In reply to comment #0)
> > > > Refactor WebViewImpl creation in preparation for supporting multiple
> > > > WebsiteDataStores
> > > 
> > > What do you mean by supporting multiple WebsiteDataStores?
> > 
> > Currently there's no concept of more than one non-default, persistent
> > WKWebsiteDataStore in the Cocoa API. There's about to be!
> > 
> > (In reply to comment #5)
> > > Comment on attachment 302316 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=302316&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:554
> > > > +        [NSException raise:NSInternalInconsistencyException format:@"[WKWebView _initializeWithConfiguration:] failed to create a new WebProcess"];
> > > 
> > > Given that _initializeWithConfiguration is an implementation detail, it
> > > seems like a bad thing to put in an exception that can be consumed by third
> > > parties.
> > 
> > Good point.
> > 
> > > 
> > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:908
> > > > +    _data->_impl = WebViewImpl::maybeCreate(self, nullptr, processPool, WTFMove(configuration));
> > > 
> > > Under what scenarios does this fail?
> > 
> > If we attempt to make a WK(Web)View with a non-default data store but we're
> > at the process cap.
> > 
> > e.g. - In practice, it won't.
> > 
> > Note: This seemingly artificial limitation is being imposed because too much
> > of the data store types are currently WebProcess-global instead of being
> > per-WK(Web)View.
> > 
> > It can be unwound in the future after more refactoring.
> 
> It seems like bumping past the limit would probably be better than throwing,
> if this will be fixed in the future.

I'll play with this

[Brady Eidson]

(In reply to comment #21)
> (In reply to comment #18)
> > (In reply to comment #6)
> > > (In reply to comment #4)
> > > It can be unwound in the future after more refactoring.
> > 
> > It seems like bumping past the limit would probably be better than throwing,
> > if this will be fixed in the future.
> 
> I'll play with this

This was a clear win for the patch.

[Brady Eidson]

Created attachment 302891 [details]
Patch

[Brady Eidson]

Created attachment 302943 [details]
Patch

[Brady Eidson]

Created attachment 302945 [details]
Patch

[Sam Weinig]

Comment on attachment 302945 [details]
Patch

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

> Source/WebKit2/UIProcess/WebProcessPool.cpp:696
> +    createNewWebProcess(nullptr);

Instead of nullptr, should this pass WebsiteDataStore::defaultDataStore()->websiteDataStore()? If you did that, could you pass around the dataStores by reference?

> Source/WebKit2/UIProcess/WebProcessPool.cpp:778
> +#if PLATFORM(COCOA)
> +    bool goOverLimit = websiteDataStore && websiteDataStore != &API::WebsiteDataStore::defaultDataStore()->websiteDataStore();
> +#else
> +    bool goOverLimit = false;
> +#endif

This could probably use a comment / bug link for it's removal.  Otherwise, patch looks good.

[Brady Eidson]

(In reply to comment #10)
> 
> > Source/WebKit2/UIProcess/WebProcessPool.cpp:760
> > +    // If a non-default WebsiteDataStore was passed in and we couldn't make a new web process for it,
> > +    // we should not return an existing process.
> 
> Is it impossible that there is an existing Web process with the same
> non-default data store?

Checking for reusing processes with the same WebsiteDataStore is what was necessary to resolve the API tests!

[Brady Eidson]

(In reply to comment #26)
> Comment on attachment 302945 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302945&action=review
> 
> > Source/WebKit2/UIProcess/WebProcessPool.cpp:696
> > +    createNewWebProcess(nullptr);
> 
> Instead of nullptr, should this pass
> WebsiteDataStore::defaultDataStore()->websiteDataStore()? If you did that,
> could you pass around the dataStores by reference?

Apparently GTK doesn't do this.

There's definitely a near-future here where that will be achieved, though.

> > Source/WebKit2/UIProcess/WebProcessPool.cpp:778
> > +#if PLATFORM(COCOA)
> > +    bool goOverLimit = websiteDataStore && websiteDataStore != &API::WebsiteDataStore::defaultDataStore()->websiteDataStore();
> > +#else
> > +    bool goOverLimit = false;
> > +#endif
> 
> This could probably use a comment / bug link for it's removal.  Otherwise,
> patch looks good.

kk

[Brady Eidson]

Created attachment 302969 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 302969 [details]
Patch

Clearing flags on attachment: 302969

Committed r213168: <http://trac.webkit.org/changeset/213168>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to comment #30)
> Comment on attachment 302969 [details]
> Patch
> 
> Clearing flags on attachment: 302969
> 
> Committed r213168: <http://trac.webkit.org/changeset/213168>

This change appears to have caused API test WKUserContentController.ScriptMessageHandlerMultipleHandlerRemoval to crash.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x000000010a7f97a2 WebKit::WebProcessPool::createNewWebProcessRespectingProcessCountLimit(WebKit::WebsiteDataStore*) + 314 (WebProcessPool.cpp:803)
1   com.apple.WebKit              	0x000000010a7f993c WebKit::WebProcessPool::createWebPage(WebKit::PageClient&, WTF::Ref<API::PageConfiguration>&&) + 364 (WebProcessPool.cpp:832)
2   com.apple.WebKit              	0x000000010a833187 WebKit::WebViewImpl::WebViewImpl(NSView<WebViewImplDelegate>*, WKWebView*, WebKit::WebProcessPool&, WTF::Ref<API::PageConfiguration>&&) + 95 (WebViewImpl.mm:1235)
3   com.apple.WebKit              	0x000000010a89103d -[WKWebView _initializeWithConfiguration:] + 3246 (WKWebView.mm:553)
4   com.apple.WebKit              	0x000000010a8916ad -[WKWebView initWithFrame:configuration:] + 109 (WKWebView.mm:607)
5   TestWebKitAPI                 	0x0000000108ea0e47 webViewForScriptMessageHandlerMultipleHandlerRemovalTest(WKWebViewConfiguration*) + 402 (UserContentController.mm:228)
6   TestWebKitAPI                 	0x0000000108ea09c7 WKUserContentController_ScriptMessageHandlerMultipleHandlerRemoval_Test::TestBody() + 271 (UserContentController.mm:246)
7   TestWebKitAPI                 	0x0000000108f032cc testing::Test::Run() + 92
8   TestWebKitAPI                 	0x0000000108f03970 testing::internal::TestInfoImpl::Run() + 178
9   TestWebKitAPI                 	0x0000000108f03d68 testing::TestCase::Run() + 188
10  TestWebKitAPI                 	0x0000000108f073f1 testing::internal::UnitTestImpl::RunAllTests() + 581
11  TestWebKitAPI                 	0x0000000108e24611 TestWebKitAPI::TestsController::run(int, char**) + 131 (TestsController.cpp:80)
12  TestWebKitAPI                 	0x0000000108efb93b main + 349 (mainMac.mm:52)
13  libdyld.dylib                 	0x7fff9d9ba5ad start + 1 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libdyld/dyld-360.22/src/start_glue.s:47)


https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/13567

[Brady Eidson]

(In reply to comment #32)
> (In reply to comment #30)
> > Comment on attachment 302969 [details]
> > Patch
> > 
> > Clearing flags on attachment: 302969
> > 
> > Committed r213168: <http://trac.webkit.org/changeset/213168>
> 
> This change appears to have caused API test
> WKUserContentController.ScriptMessageHandlerMultipleHandlerRemoval to crash.
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebKit              	0x000000010a7f97a2
> WebKit::WebProcessPool::
> createNewWebProcessRespectingProcessCountLimit(WebKit::WebsiteDataStore*) +
> 314 (WebProcessPool.cpp:803)
> 1   com.apple.WebKit              	0x000000010a7f993c
> WebKit::WebProcessPool::createWebPage(WebKit::PageClient&,
> WTF::Ref<API::PageConfiguration>&&) + 364 (WebProcessPool.cpp:832)
> 2   com.apple.WebKit              	0x000000010a833187
> WebKit::WebViewImpl::WebViewImpl(NSView<WebViewImplDelegate>*, WKWebView*,
> WebKit::WebProcessPool&, WTF::Ref<API::PageConfiguration>&&) + 95
> (WebViewImpl.mm:1235)
> 3   com.apple.WebKit              	0x000000010a89103d -[WKWebView
> _initializeWithConfiguration:] + 3246 (WKWebView.mm:553)
> 4   com.apple.WebKit              	0x000000010a8916ad -[WKWebView
> initWithFrame:configuration:] + 109 (WKWebView.mm:607)
> 5   TestWebKitAPI                 	0x0000000108ea0e47
> webViewForScriptMessageHandlerMultipleHandlerRemovalTest(WKWebViewConfigurati
> on*) + 402 (UserContentController.mm:228)
> 6   TestWebKitAPI                 	0x0000000108ea09c7
> WKUserContentController_ScriptMessageHandlerMultipleHandlerRemoval_Test::
> TestBody() + 271 (UserContentController.mm:246)
> 7   TestWebKitAPI                 	0x0000000108f032cc testing::Test::Run() +
> 92
> 8   TestWebKitAPI                 	0x0000000108f03970
> testing::internal::TestInfoImpl::Run() + 178
> 9   TestWebKitAPI                 	0x0000000108f03d68
> testing::TestCase::Run() + 188
> 10  TestWebKitAPI                 	0x0000000108f073f1
> testing::internal::UnitTestImpl::RunAllTests() + 581
> 11  TestWebKitAPI                 	0x0000000108e24611
> TestWebKitAPI::TestsController::run(int, char**) + 131
> (TestsController.cpp:80)
> 12  TestWebKitAPI                 	0x0000000108efb93b main + 349
> (mainMac.mm:52)
> 13  libdyld.dylib                 	0x7fff9d9ba5ad start + 1
> (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libdyld/dyld-360.22/src/
> start_glue.s:47)
> 
> 
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/13567


Was resolved in https://trac.webkit.org/changeset/213181