Bug 237065 – [macOS] TestWebKitAPI.WebKit.MigrateLocalStorageDataToGeneralStorageDirectory is a flaky failure

• Status: RESOLVED
• Resolution: FIXED
• Component: Website Storage

[Ryan Haddad]

TestWebKitAPI.WebKit.MigrateLocalStorageDataToGeneralStorageDirectory, added with https://commits.webkit.org/247316@main, is a flaky failure on macOS bots

    TestWebKitAPI.WebKit.MigrateLocalStorageDataToGeneralStorageDirectory
        LEAK: 1 WebProcessPool
        
        /Volumes/Data/worker/monterey-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:864
        Expected equality of these values:
          "testvalue"
          getNextMessage().body
            Which is: "[null]"


https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WebKit.MigrateLocalStorageDataToGeneralStorageDirectory

[Radar WebKit Bug Importer]

<rdar://problem/89324250>

[Sihui Liu]

Created attachment 453079 [details]
Patch

[Sihui Liu]

Created attachment 453229 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 453229 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:264
>      // Make sure we flush all cookies to disk before exiting.

This comment should probably be omitted. Typically, we want comments to explain "why", not "what". This comment just explains what, and it's not even entirely accurate any more after this addition.

> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:89
> +        std::atexit(commitTransactionsAtExit);

Do we still have a boost to prevent suspension at this point?

[Sihui Liu]

Comment on attachment 453229 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:264
>>      // Make sure we flush all cookies to disk before exiting.
> 
> This comment should probably be omitted. Typically, we want comments to explain "why", not "what". This comment just explains what, and it's not even entirely accurate any more after this addition.

Sure, will remove.

>> Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp:89
>> +        std::atexit(commitTransactionsAtExit);
> 
> Do we still have a boost to prevent suspension at this point?

You mean to prevent process from being suspended by taking an assertion?
I am not sure if a process can get suspended when it is exiting, but if it can, the worst case seems to be the process getting killed and losing data in unfinished transactions, which sounds like a rare case.

[Sihui Liu]

Created attachment 453284 [details]
Patch for landing

[EWS]

Committed r290544 (247822@main): <https://commits.webkit.org/247822@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453284 [details].

[Sihui Liu]

Reopened as the test failure is not fixed.

[Sihui Liu]

Created attachment 453368 [details]
Patch

[Darin Adler]

Comment on attachment 453368 [details]
Patch

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

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:43
> +#include <wtf/WeakHashSet.h>

Should not need this in the header just to declare a function returning a reference to a WeakHashSet. Forward.h should suffice.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:74
> +    static WeakHashSet<NetworkStorageManager>& allNetworkStorageManagers();

I suggest we publish a forEachNetworkStorageManager function for NetworkProcess.cpp to use instead of of publishing a read/write reference to a WeakHashSet; would be nice to keep the WeakHashSet as a hidden implementation detail of this class.

[Sihui Liu]

(In reply to Darin Adler from comment #10)
> Comment on attachment 453368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453368&action=review
> 
> > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:43
> > +#include <wtf/WeakHashSet.h>
> 
> Should not need this in the header just to declare a function returning a
> reference to a WeakHashSet. Forward.h should suffice.
> 
> > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:74
> > +    static WeakHashSet<NetworkStorageManager>& allNetworkStorageManagers();
> 
> I suggest we publish a forEachNetworkStorageManager function for
> NetworkProcess.cpp to use instead of of publishing a read/write reference to
> a WeakHashSet; would be nice to keep the WeakHashSet as a hidden
> implementation detail of this class.

Sure, will add NetworkStorageManager::forEach() and remove WeakHashSet.h.

[Sihui Liu]

Created attachment 453449 [details]
Patch for landing

[Darin Adler]

Comment on attachment 453449 [details]
Patch for landing

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

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:43
> +#include <wtf/WeakHashSet.h>

Please don’t add this include to this header.

[EWS]

Committed r290623 (247896@main): <https://commits.webkit.org/247896@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453449 [details].

[Sihui Liu]

Comment on attachment 453449 [details]
Patch for landing

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

>> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h:43
>> +#include <wtf/WeakHashSet.h>
> 
> Please don’t add this include to this header.

Oops, I missed this. Removed in r290669.