Multiple API failures are happening consistently after https://trac.webkit.org/changeset/225789/webkit WKProcessPool.InitialWarmedProcessUsed WebKit.WebsiteDataStoreCustomPaths https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20%28Tests%29/builds/6953/steps/run-api-tests/logs/stdio Build: https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20%28Tests%29/builds/6953/steps/run-api-tests/logs/stdio FAIL WKProcessPool.InitialWarmedProcessUsed /Volumes/Data/slave/elcapitan-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/InitialWarmedProcessUsed.mm:51 Value of: static_cast<size_t>(1) Actual: 1 Expected: [pool _webProcessCount] Which is: 2 FAIL WebKit.WebsiteDataStoreCustomPaths /Volumes/Data/slave/elcapitan-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:219 Value of: [WKWebsiteDataStore _defaultDataStoreExists] Actual: true Expected: false
<rdar://problem/36012825>
Enabling SW for API tests caused two changes: 1 - An additional WebProcess exists, though it doesn't host WebPages. 2 - We accidentally create the default WebsiteDataStore even when only a custom store is used. #1 is resolved with a testing change, and #2 is a real bug to be fixed.
Created attachment 329225 [details] Patch
Comment on attachment 329225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329225&action=review r=me with comments. > Source/WebKit/StorageProcess/StorageProcess.cpp:437 > + parentProcessConnection()->send(Messages::StorageProcessProxy::EstablishWorkerContextConnectionToStorageProcessForExplicitSession(*sessionID), 0); There is no reason we cannot have a single IPC message that takes in a std::optional<PAL::SessionID>, is there? > Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:447 > + bool serviceWorkerProcessWasFound = false; I think this whole block should be protected by #if !ASSERT_DISABLED. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:60 > + static NeverDestroyed<HashMap<PAL::SessionID, WebsiteDataStore*>> map; We may want to add a RELEASE_ASSERT to make sure this is always called on the main thread. I know how clients like to use our API from various threads and such HashMap could get corrupted. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:99 > + ASSERT(allDataStores().get(m_sessionID) == this); remove() returns a boolean, I would have been more efficient to ASSERT_UNUSED(wasRemoved, wasRemoved); after the remove() call.
(In reply to Chris Dumez from comment #4) > Comment on attachment 329225 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=329225&action=review > > r=me with comments. > > > Source/WebKit/StorageProcess/StorageProcess.cpp:437 > > + parentProcessConnection()->send(Messages::StorageProcessProxy::EstablishWorkerContextConnectionToStorageProcessForExplicitSession(*sessionID), 0); > > There is no reason we cannot have a single IPC message that takes in a > std::optional<PAL::SessionID>, is there? While debugging I immediately found it useful to see a different message name here. (This is occasionally true throughout WK2) > > > Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:447 > > + bool serviceWorkerProcessWasFound = false; > > I think this whole block should be protected by #if !ASSERT_DISABLED. Yah, probably true. > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:60 > > + static NeverDestroyed<HashMap<PAL::SessionID, WebsiteDataStore*>> map; > > We may want to add a RELEASE_ASSERT to make sure this is always called on > the main thread. I know how clients like to use our API from various threads > and such HashMap could get corrupted. Sure. > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:99 > > + ASSERT(allDataStores().get(m_sessionID) == this); > > remove() returns a boolean, I would have been more efficient to > ASSERT_UNUSED(wasRemoved, wasRemoved); after the remove() call. k
(In reply to Brady Eidson from comment #5) > (In reply to Chris Dumez from comment #4) > > > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:99 > > > + ASSERT(allDataStores().get(m_sessionID) == this); > > > > remove() returns a boolean, I would have been more efficient to > > ASSERT_UNUSED(wasRemoved, wasRemoved); after the remove() call. > > k Actually, still prefer this version as it verifies not only that the entry exists, but that the entry is "this"
Created attachment 329233 [details] Patch
Comment on attachment 329233 [details] Patch Clearing flags on attachment: 329233 Committed r225864: <https://trac.webkit.org/changeset/225864>
All reviewed patches have been landed. Closing bug.
This patch caused the layout test step in the testers to crash, fail, and unable to run. https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r225872%20(1241)/results.html https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/1241 Link to crash: https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r225872%20(1241)/accessibility/attachment-element-crash-log.txt stderr: LEAK: 1 WebPage LEAK: 1 WebFrame LEAK: 69 RenderObject LEAK: 4191 BidiRun LEAK: 9 Page LEAK: 9 Frame LEAK: 109 CachedResource LEAK: 630 WebCoreNode ASSERTION FAILED: result.isNewEntry /Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp(82) : WebKit::WebsiteDataStore::WebsiteDataStore(WebKit::WebsiteDataStore::Configuration, PAL::SessionID) 1 0x10fb48c7d WTFCrash 2 0x1137648a7 WebKit::WebsiteDataStore::WebsiteDataStore(WebKit::WebsiteDataStore::Configuration, PAL::SessionID) 3 0x1137646ed WebKit::WebsiteDataStore::WebsiteDataStore(WebKit::WebsiteDataStore::Configuration, PAL::SessionID) 4 0x1137646a2 WebKit::WebsiteDataStore::create(WebKit::WebsiteDataStore::Configuration, PAL::SessionID) 5 0x112a807e7 API::WebsiteDataStore::WebsiteDataStore(WebKit::WebsiteDataStore::Configuration, PAL::SessionID) 6 0x112a8043d API::WebsiteDataStore::WebsiteDataStore(WebKit::WebsiteDataStore::Configuration, PAL::SessionID) 7 0x112a8062b API::WebsiteDataStore::createLegacy(WebKit::WebsiteDataStore::Configuration) 8 0x11369da11 WebKit::WebProcessPool::WebProcessPool(API::ProcessPoolConfiguration&) 9 0x11369c0fd WebKit::WebProcessPool::WebProcessPool(API::ProcessPoolConfiguration&) 10 0x11369c068 WebKit::WebProcessPool::create(API::ProcessPoolConfiguration&) 11 0x1138b9771 WKContextCreateWithConfiguration 12 0x10e4f88e7 WTR::TestController::generatePageConfiguration(OpaqueWKContextConfiguration const*) 13 0x10e4f96e0 WTR::TestController::createWebViewWithOptions(WTR::TestOptions const&) 14 0x10e4fa9ea WTR::TestController::ensureViewSupportsOptionsForTest(WTR::TestInvocation const&) 15 0x10e500d1f WTR::TestController::configureViewForTest(WTR::TestInvocation const&) 16 0x10e523dee WTR::TestInvocation::invoke() 17 0x10e504d5d WTR::TestController::runTest(char const*) 18 0x10e505ed4 WTR::TestController::runTestingServerLoop() 19 0x10e4f6836 WTR::TestController::run() 20 0x10e4f61ba WTR::TestController::TestController(int, char const**) 21 0x10e4f69f3 WTR::TestController::TestController(int, char const**) 22 0x10e4d5def main 23 0x7fff584d2115 start
Created attachment 329276 [details] Crash caused by first patch Attaching crash
Reverted r225864 for reason: This caused the Layout test step to crash out on Debug testers Committed r225877: <https://trac.webkit.org/changeset/225877>
(In reply to Matt Lewis from comment #10) > This patch caused the layout test step in the testers to crash, fail, and > unable to run. > > https://build.webkit.org/results/ > Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r225872%20(1241)/results.html > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/1241 > > Link to crash: > https://build.webkit.org/results/ > Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r225872%20(1241)/accessibility/ > attachment-element-crash-log.txt > Why did EWS not catch that? *sigh*
Easy to reproduce locally with: run-webkit-tests aria-used-on-image-maps.html attachment-element.html Testing a fix locally before uploading.
Created attachment 329303 [details] Patch
This new patch is the same, but only tracks non-default data stores. (Multiple data store objects can be made to represent the default data store, but that's not true for non-default ones)
Can this patch get landed as-is or does it need another review?
(In reply to Ryan Haddad from comment #17) > Can this patch get landed as-is or does it need another review? Still r=me.
Comment on attachment 329303 [details] Patch Marking cq+ to get the bots back to green.
Comment on attachment 329303 [details] Patch Clearing flags on attachment: 329303 Committed r225935: <https://trac.webkit.org/changeset/225935>
Yah sorry I was AFK most the day, I meant to cq+ once EWS was green (It was taking its sweet time last night) Thanks for doing all the right things!
One of the API tests is still failing after the fix: FAIL WebKit.WebsiteDataStoreCustomPaths /Volumes/Data/slave/elcapitan-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:219 Value of: [WKWebsiteDataStore _defaultDataStoreExists] Actual: true Expected: false I can reproduce the failure locally on High Sierra.
(In reply to Ryan Haddad from comment #23) > One of the API tests is still failing after the fix: > FAIL WebKit.WebsiteDataStoreCustomPaths > > /Volumes/Data/slave/elcapitan-release/build/Tools/TestWebKitAPI/Tests/ > WebKitCocoa/WebsiteDataStoreCustomPaths.mm:219 > Value of: [WKWebsiteDataStore _defaultDataStoreExists] > Actual: true > Expected: false > > I can reproduce the failure locally on High Sierra. Filed https://bugs.webkit.org/show_bug.cgi?id=180870