Bug 209016 – [ iOS and Mac wk2 ] http/tests/in-app-browser-privacy/ tests failing.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

These started failing again after fixing <rdar://problem/60250973>

[Kate Cheney]

<rdar://problem/60329530>

[Kate Cheney]

Created attachment 393414 [details]
Patch

[Kate Cheney]

Created attachment 393417 [details]
Patch

[Chris Dumez]

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.

[Kate Cheney]

(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

[Kate Cheney]

Created attachment 393519 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

(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();
}

[Kate Cheney]

(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

[Kate Cheney]

Created attachment 393525 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 393525 [details]
Patch

Clearing flags on attachment: 393525

Committed r258436: <https://trac.webkit.org/changeset/258436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60435282>