These started failing again after fixing <rdar://problem/60250973>
<rdar://problem/60329530>
Created attachment 393414 [details] Patch
Created attachment 393417 [details] Patch
Comment on attachment 393417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393417&action=review > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:168 > +WK_EXPORT void WKWebsiteDataStoreReinitializeAppBoundDomains(WKWebsiteDataStoreRef dataStoreRef, void* context, WKWebsiteDataStoreReinitializeAppBoundDomainsFunction completionHandler); This does not seem to need to completion handler since it is sync. > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:515 > +void WebsiteDataStore::reinitializeAppBoundDomains(CompletionHandler<void()>&& completionHandler) This does not seem to need to completion handler since it is sync. > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:518 > + initializeAppBoundDomains(); What clears existing app bound domains? It seems initializeAppBoundDomains() will just keep adding more but I do not think anything doing the clearing.
(In reply to Chris Dumez from comment #4) > Comment on attachment 393417 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393417&action=review > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:168 > > +WK_EXPORT void WKWebsiteDataStoreReinitializeAppBoundDomains(WKWebsiteDataStoreRef dataStoreRef, void* context, WKWebsiteDataStoreReinitializeAppBoundDomainsFunction completionHandler); > > This does not seem to need to completion handler since it is sync. > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:515 > > +void WebsiteDataStore::reinitializeAppBoundDomains(CompletionHandler<void()>&& completionHandler) > > This does not seem to need to completion handler since it is sync. > Great! I will remove this. > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:518 > > + initializeAppBoundDomains(); > > What clears existing app bound domains? It seems initializeAppBoundDomains() > will just keep adding more but I do not think anything doing the clearing. Good point, I will change this to clear before re-initializing
Created attachment 393519 [details] Patch
Comment on attachment 393519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393519&action=review > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:527 > + hasInitializedAppBoundDomains = false; I think that for this code to actually be correct, you'd have to reset hasInitializedAppBoundDomains to false in the lambda inside clearAppBoundDomains(), after clearing the domains. That said, I think we can simplify this code a lot like by tweaking existing code like so: enum class ForceReinitialization : bool { No, Yes }; void WebsiteDataStore::initializeAppBoundDomains(ForceReinitialization forceReinitialization) { ASSERT(RunLoop::isMain()); if (hasInitializedAppBoundDomains && forceReinitialization != ForceReinitialization::Yes) // Updated return; static const auto maxAppBoundDomainCount = 10; appBoundDomainQueue().dispatch([forceReinitialization] () mutable { if (hasInitializedAppBoundDomains && forceReinitialization != ForceReinitialization::Yes) // Updated return; NSArray<NSString *> *domains = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"WKAppBoundDomains"]; RunLoop::main().dispatch([forceReinitialization , domains = retainPtr(domains)] { if ( forceReinitialization == ForceReinitialization::Yes) // New appBoundDomains().clear(); // New for (NSString *domain in domains.get()) { URL url { URL(), domain }; if (!url.isValid()) continue; WebCore::RegistrableDomain appBoundDomain { url }; if (appBoundDomain.isEmpty()) continue; appBoundDomains().add(appBoundDomain); if (appBoundDomains().size() >= maxAppBoundDomainCount) break; } WEBSITE_DATA_STORE_ADDITIONS hasInitializedAppBoundDomains = true; }); }); } Then you only have to call initializeAppBoundDomains(ForceReinitialization::Yes) in your new reinitializeAppBoundDomains() method. What do you think?
(In reply to Chris Dumez from comment #7) > Comment on attachment 393519 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393519&action=review > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:527 > > + hasInitializedAppBoundDomains = false; > > I think that for this code to actually be correct, you'd have to reset > hasInitializedAppBoundDomains to false in the lambda inside > clearAppBoundDomains(), after clearing the domains. That said, I think we > can simplify this code a lot like by tweaking existing code like so: > > enum class ForceReinitialization : bool { No, Yes }; > void WebsiteDataStore::initializeAppBoundDomains(ForceReinitialization > forceReinitialization) > { > ASSERT(RunLoop::isMain()); > > if (hasInitializedAppBoundDomains && forceReinitialization != > ForceReinitialization::Yes) // Updated > return; > > static const auto maxAppBoundDomainCount = 10; > > appBoundDomainQueue().dispatch([forceReinitialization] () mutable { > if (hasInitializedAppBoundDomains && forceReinitialization != > ForceReinitialization::Yes) // Updated > return; > > NSArray<NSString *> *domains = [[NSBundle mainBundle] > objectForInfoDictionaryKey:@"WKAppBoundDomains"]; > > RunLoop::main().dispatch([forceReinitialization , domains = > retainPtr(domains)] { > if ( forceReinitialization == ForceReinitialization::Yes) // New > appBoundDomains().clear(); // New > > for (NSString *domain in domains.get()) { > URL url { URL(), domain }; > if (!url.isValid()) > continue; > WebCore::RegistrableDomain appBoundDomain { url }; > if (appBoundDomain.isEmpty()) > continue; > appBoundDomains().add(appBoundDomain); > if (appBoundDomains().size() >= maxAppBoundDomainCount) > break; > } > WEBSITE_DATA_STORE_ADDITIONS > hasInitializedAppBoundDomains = true; > }); > }); > } > > Then you only have to call > initializeAppBoundDomains(ForceReinitialization::Yes) in your new > reinitializeAppBoundDomains() method. > > What do you think? Actually, I think you have to reset hasInitializedAppBoundDomains synchronously to false on the main thread to avoid flakiness. Here is a counter proposal then: void WebsiteDataStore::reinitializeAppBoundDomains() { if (!hasInitializedAppBoundDomains) return; appBoundDomains().clear(); hasInitializedAppBoundDomains = false; initializeAppBoundDomains(); }
(In reply to Chris Dumez from comment #7) > Comment on attachment 393519 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393519&action=review > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:527 > > + hasInitializedAppBoundDomains = false; > > I think that for this code to actually be correct, you'd have to reset > hasInitializedAppBoundDomains to false in the lambda inside > clearAppBoundDomains(), after clearing the domains. That said, I think we > can simplify this code a lot like by tweaking existing code like so: > > enum class ForceReinitialization : bool { No, Yes }; > void WebsiteDataStore::initializeAppBoundDomains(ForceReinitialization > forceReinitialization) > { > ASSERT(RunLoop::isMain()); > > if (hasInitializedAppBoundDomains && forceReinitialization != > ForceReinitialization::Yes) // Updated > return; > > static const auto maxAppBoundDomainCount = 10; > > appBoundDomainQueue().dispatch([forceReinitialization] () mutable { > if (hasInitializedAppBoundDomains && forceReinitialization != > ForceReinitialization::Yes) // Updated > return; > > NSArray<NSString *> *domains = [[NSBundle mainBundle] > objectForInfoDictionaryKey:@"WKAppBoundDomains"]; > > RunLoop::main().dispatch([forceReinitialization , domains = > retainPtr(domains)] { > if ( forceReinitialization == ForceReinitialization::Yes) // New > appBoundDomains().clear(); // New > > for (NSString *domain in domains.get()) { > URL url { URL(), domain }; > if (!url.isValid()) > continue; > WebCore::RegistrableDomain appBoundDomain { url }; > if (appBoundDomain.isEmpty()) > continue; > appBoundDomains().add(appBoundDomain); > if (appBoundDomains().size() >= maxAppBoundDomainCount) > break; > } > WEBSITE_DATA_STORE_ADDITIONS > hasInitializedAppBoundDomains = true; > }); > }); > } > > Then you only have to call > initializeAppBoundDomains(ForceReinitialization::Yes) in your new > reinitializeAppBoundDomains() method. > > What do you think? This looks good! With the addition of setting hasInitializedAppBoundDomains = false before calling this in reinitializeAppBoundDomains
Created attachment 393525 [details] Patch
Comment on attachment 393525 [details] Patch Clearing flags on attachment: 393525 Committed r258436: <https://trac.webkit.org/changeset/258436>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60435282>