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
<rdar://problem/89324250>
Created attachment 453079 [details] Patch
Created attachment 453229 [details] Patch
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?
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.
Created attachment 453284 [details] Patch for landing
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].
Reopened as the test failure is not fixed.
Created attachment 453368 [details] Patch
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.
(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.
Created attachment 453449 [details] Patch for landing
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.
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].
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.