REGRESSION (r225789): API tests WKProcessPool.InitialWarmedProcessUsed and WebKit.WebsiteDataStoreCustomPaths are failing

• Component: New Bugs
• Status: RESOLVED FIXED

[Matt Lewis]

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

[Radar WebKit Bug Importer]

<rdar://problem/36012825>

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 329225 [details]
Patch

[Chris Dumez]

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.

[Brady Eidson]

(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

[Brady Eidson]

(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"

[Brady Eidson]

Created attachment 329233 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329233 [details]
Patch

Clearing flags on attachment: 329233

Committed r225864: <https://trac.webkit.org/changeset/225864>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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

[Matt Lewis]

Created attachment 329276 [details]
Crash caused by first patch

Attaching crash

[Matt Lewis]

Reverted r225864 for reason:

This caused the Layout test step to crash out on Debug testers

Committed r225877: <https://trac.webkit.org/changeset/225877>

[Brady Eidson]

(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*

[Brady Eidson]

Easy to reproduce locally with:

run-webkit-tests aria-used-on-image-maps.html attachment-element.html

Testing a fix locally before uploading.

[Brady Eidson]

Created attachment 329303 [details]
Patch

[Brady Eidson]

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)

[Ryan Haddad]

Can this patch get landed as-is or does it need another review?

[Chris Dumez]

(In reply to Ryan Haddad from comment #17)
> Can this patch get landed as-is or does it need another review?

Still r=me.

[Ryan Haddad]

Comment on attachment 329303 [details]
Patch

Marking cq+ to get the bots back to green.

[WebKit Commit Bot]

Comment on attachment 329303 [details]
Patch

Clearing flags on attachment: 329303

Committed r225935: <https://trac.webkit.org/changeset/225935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

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!

[Ryan Haddad]

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.

[Ryan Haddad]

(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