Bug 209016 - [ iOS and Mac wk2 ] http/tests/in-app-browser-privacy/ tests failing.
Summary: [ iOS and Mac wk2 ] http/tests/in-app-browser-privacy/ tests failing.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-12 13:44 PDT by Kate Cheney
Modified: 2020-03-13 14:58 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.93 KB, patch)
2020-03-12 14:32 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (10.00 KB, patch)
2020-03-12 14:55 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (10.06 KB, patch)
2020-03-13 12:43 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (11.11 KB, patch)
2020-03-13 13:53 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2020-03-12 13:44:38 PDT
These started failing again after fixing <rdar://problem/60250973>
Comment 1 Kate Cheney 2020-03-12 13:45:24 PDT
<rdar://problem/60329530>
Comment 2 Kate Cheney 2020-03-12 14:32:21 PDT
Created attachment 393414 [details]
Patch
Comment 3 Kate Cheney 2020-03-12 14:55:19 PDT
Created attachment 393417 [details]
Patch
Comment 4 Chris Dumez 2020-03-13 10:46:39 PDT
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.
Comment 5 Kate Cheney 2020-03-13 10:48:48 PDT
(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
Comment 6 Kate Cheney 2020-03-13 12:43:44 PDT
Created attachment 393519 [details]
Patch
Comment 7 Chris Dumez 2020-03-13 13:05:19 PDT
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?
Comment 8 Chris Dumez 2020-03-13 13:13:50 PDT
(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();
}
Comment 9 Kate Cheney 2020-03-13 13:32:47 PDT
(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
Comment 10 Kate Cheney 2020-03-13 13:53:43 PDT
Created attachment 393525 [details]
Patch
Comment 11 WebKit Commit Bot 2020-03-13 14:57:01 PDT
Comment on attachment 393525 [details]
Patch

Clearing flags on attachment: 393525

Committed r258436: <https://trac.webkit.org/changeset/258436>
Comment 12 WebKit Commit Bot 2020-03-13 14:57:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2020-03-13 14:58:17 PDT
<rdar://problem/60435282>