| Summary: | Refactor WebViewImpl creation in preparation for supporting multiple WebsiteDataStores | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||||
| Component: | WebKit Misc. | Assignee: | Brady Eidson <beidson> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | andersca, cgarcia, commit-queue, mitz, ryanhaddad, sam, thorton | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=168983 | ||||||||||||||||||
| Bug Depends on: | 168710 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Brady Eidson
2017-02-21 14:38:41 PST
Created attachment 302311 [details]
Patch
Created attachment 302316 [details]
Patch
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 (In reply to comment #0) > Refactor WebViewImpl creation in preparation for supporting multiple > WebsiteDataStores What do you mean by supporting multiple WebsiteDataStores? 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? (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. Created attachment 302341 [details]
Patch
Comment on attachment 302341 [details] Patch Clearing flags on attachment: 302341 Committed r212786: <http://trac.webkit.org/changeset/212786> 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? (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. 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. 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. 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. 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. Re-opened since this is blocked by bug 168710 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 (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. (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. (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 (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 (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. Created attachment 302891 [details]
Patch
Created attachment 302943 [details]
Patch
Created attachment 302945 [details]
Patch
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. (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! (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 Created attachment 302969 [details]
Patch
Comment on attachment 302969 [details] Patch Clearing flags on attachment: 302969 Committed r213168: <http://trac.webkit.org/changeset/213168> All reviewed patches have been landed. Closing bug. (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 (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 |