Summary: | [ iOS and Mac wk2 ] http/tests/in-app-browser-privacy/ tests failing. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, cdumez, commit-queue, webkit-bug-importer, wilander | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Kate Cheney
2020-03-12 13:44:38 PDT
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. |