Bug 198090 – API Test landed in r245540 [Mac WK2] TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure

• Component: WebKit API
• Status: RESOLVED FIXED

[Shawn Roberts]

API test added in https://trac.webkit.org/changeset/245540/webkit

TestWebKitAPI.WKWebView.LocalStorageProcessCrashes is a flaky failure on Mac Release WK2 testers

https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/4242/steps/run-api-tests/logs/stdio

Reproduced with:

run-api-tests TestWebKitAPI.WKWebView.LocalStorageProcessCrashes --debug --iter30

    TestWebKitAPI.WKWebView.LocalStorageProcessCrashes
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        
        /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:91
        Value of: [@"storage" isEqualToString:result]
          Actual: false
        Expected: true

[Radar WebKit Bug Importer]

<rdar://problem/51003644>

[Sihui Liu]

Created attachment 370382 [details]
Patch

[youenn fablet]

Comment on attachment 370382 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        thread and main thread does not know how to decode it.

Looking at existing worker queue code, we would probably need to register it at network process start time to make it work.
That would probably be a major change.

Another approach might be to send a sync IPC when recovering from the network process crash.
This might be a smaller change and we anyway want to change how we handle crash recovery.

But all of this would strengthen the fact that one IPC connection is related to one session ID and we want to move away from this.
(Or we would need to do the sessionID dispatching in the background thread).
It is safer and simpler to rely on main thread IPC and do the dispatching ourselves, even though this will be less efficient.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:714
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, storageMapID, storageNamespaceID, securityOriginData = WTFMove(securityOriginData)]() mutable {

In theory we should isolatedCopy() securityOriginData.
Also, what makes sure connection is alive when dispatched?
Shouldn't we ref it or just capture its uniqueID?

Ditto elsewhere for all string related parameters.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:852
> +        connection.send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);

We receive a completionHandler from the main thread.
We should probably call it from the main thread, which would mean isolatedCopy storageArea->items().
Since we use connection, we need to ref it as well.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:-980
> -    m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, &decoder] {

This code is removed, which is probably nice.
How can we be sure that connection and decoder are kept alive after dispatching to the queue?

[Sihui Liu]

(In reply to youenn fablet from comment #3)
> Comment on attachment 370382 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370382&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        thread and main thread does not know how to decode it.
> 
> Looking at existing worker queue code, we would probably need to register it
> at network process start time to make it work.
> That would probably be a major change.
> 
> Another approach might be to send a sync IPC when recovering from the
> network process crash.
> This might be a smaller change and we anyway want to change how we handle
> crash recovery.
> 
> But all of this would strengthen the fact that one IPC connection is related
> to one session ID and we want to move away from this.
> (Or we would need to do the sessionID dispatching in the background thread).
> It is safer and simpler to rely on main thread IPC and do the dispatching
> ourselves, even though this will be less efficient.
> 
Agree, will add this explanation to change log.

> > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:714
> > +    m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, storageMapID, storageNamespaceID, securityOriginData = WTFMove(securityOriginData)]() mutable {
> 
> In theory we should isolatedCopy() securityOriginData.
> Also, what makes sure connection is alive when dispatched?
> Shouldn't we ref it or just capture its uniqueID?
> 
> Ditto elsewhere for all string related parameters.
> 
Yes, we should use connection's uniqueID and isolatedCopy(). Will change.

> > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:852
> > +        connection.send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
> 
> We receive a completionHandler from the main thread.
> We should probably call it from the main thread, which would mean
> isolatedCopy storageArea->items().
> Since we use connection, we need to ref it as well.
> 
True. Will change.

> > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:-980
> > -    m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, &decoder] {
> 
> This code is removed, which is probably nice.
> How can we be sure that connection and decoder are kept alive after
> dispatching to the queue?

We cannot, especially decoder which is not copyable and will be released as soon as the message is dispatched to NetworkConnectionToWebProcess. So we have to make this change.

[Sihui Liu]

Created attachment 370424 [details]
Patch

[Sihui Liu]

Created attachment 370430 [details]
Patch

[youenn fablet]

Comment on attachment 370430 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:842
> +    RunLoop::main().dispatch([this, protectedThis = makeRef(*this), &connection, storageMapID, items = crossThreadCopy(items)] {

We should ref here.
It might be simpler to move the completion handler to background thread then main frame instead of having this map.
In that case, we need to ensure the handler is destroyed in main thread.

If sticking with the map, the key should be an incrémented integer instead of a 
There might be in theory more than one getValues call from a given connection.

[Sihui Liu]

(In reply to youenn fablet from comment #7)
> Comment on attachment 370430 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370430&action=review
> 
> > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:842
> > +    RunLoop::main().dispatch([this, protectedThis = makeRef(*this), &connection, storageMapID, items = crossThreadCopy(items)] {
> 
> We should ref here.
> It might be simpler to move the completion handler to background thread then
> main frame instead of having this map.
> In that case, we need to ensure the handler is destroyed in main thread.
> 
> If sticking with the map, the key should be an incrémented integer instead
> of a 
> There might be in theory more than one getValues call from a given
> connection.

Okay, changed to use move.

[Sihui Liu]

Created attachment 370439 [details]
Patch

[youenn fablet]

Comment on attachment 370439 [details]
Patch

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

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:741
> +    m_queue->dispatch([this, protectedThis = makeRef(*this), &connection, storageMapID, storageNamespaceID, topLevelOriginData = topLevelOriginData.isolatedCopy(), origin = origin.isolatedCopy()]() mutable {

s/&connection/connectionID = connection->uniqueID()/

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:839
> +void StorageManager::didGetValues(IPC::Connection& connection, uint64_t storageMapID, const HashMap<String, String>& items, GetValuesCallback&& completionHandler)

No need for connection.
No need for this as well, this can be a static free function.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:859
> +        connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);

This is a strange design to have both a completion handler and sending in some cases another IPC message.
It might be interesting to try simplifying this in a follow-up patch.

For instance, why do we not send back such message in the ephemeral case?
Or could we not send back storageMapSeed in the completion handler as an optional value?

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:925
> +        connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);

It seems strange to not send back StorageAreaMap::DidClear in the ephemeral case since we are clearing some data apparently.
Even in the case where the page has been closed, it seems good practice to send back some information that the task is completed.
We could modernize it with Async IPC reply, in which case we would need to call the completion handler in every case.

> Source/WebKit/NetworkProcess/WebStorage/StorageManager.h:46
> +typedef CompletionHandler<void(const HashMap<String, String>&)> GetValuesCallback;

"using" is the preferred way.

[Sihui Liu]

Created attachment 370445 [details]
Patch for landing

[Sihui Liu]

> > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:859
> > +        connection->send(Messages::StorageAreaMap::DidGetValues(storageMapSeed), storageMapID);
> 
> This is a strange design to have both a completion handler and sending in
> some cases another IPC message.
> It might be interesting to try simplifying this in a follow-up patch.
> 
> For instance, why do we not send back such message in the ephemeral case?
> Or could we not send back storageMapSeed in the completion handler as an
> optional value?
> 

I also don't understand why we need to send another didGetValues message here. Should probably either making the getValues message async or removing didGetValues message.

> > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:925
> > +        connection->send(Messages::StorageAreaMap::DidClear(storageMapSeed), storageMapID);
> 
> It seems strange to not send back StorageAreaMap::DidClear in the ephemeral
> case since we are clearing some data apparently.
> Even in the case where the page has been closed, it seems good practice to
> send back some information that the task is completed.
> We could modernize it with Async IPC reply, in which case we would need to
> call the completion handler in every case.
> 

Yes, web process may need to be notified about the result to better keep sync with network process. We used to not care much about synchronization of session storage between processes because session storage can disappear when web process shuts down. This may not be the case with PSON.

[WebKit Commit Bot]

Comment on attachment 370445 [details]
Patch for landing

Clearing flags on attachment: 370445

Committed r245649: <https://trac.webkit.org/changeset/245649>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.