Bug 217544 – Add debug logging for app-bound domains to gather telemetry on script evaluation

• Status: NEW
• Component: WebKit Misc.

[Kate Cheney]

We should add some logging to gather telemetry on script injection for app-bound domains.

[Kate Cheney]

<rdar://problem/70111380>

[Kate Cheney]

Created attachment 411152 [details]
Patch

[Kate Cheney]

Created attachment 411155 [details]
Patch

[Kate Cheney]

Created attachment 411171 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 411171 [details]
Patch

I don't think it's necessary to land this in OpenSource.

[Sam Weinig]

Comment on attachment 411171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411171&action=review

> Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.cpp:223
> +    for (auto& process : m_processes) {
> +#if ENABLE(APP_BOUND_DOMAINS)
> +        if (appBoundDomainsLoggingEnabled)
> +            logInBatches(process, userScript.userScript().source());
> +#endif
>          process.send(Messages::WebUserContentController::AddUserScripts({ { userScript.identifier(), world->identifier(), userScript.userScript() } }, immediately), identifier());
> +    }

Sounds like this might not actually need to get landed, but none-the-less, if you were going to land something like this, I would recommend going with something like this:

static void logScriptIfAppropriate(const WeakHashSet<WebProcessProxy>& processes, StringView userScript)
{
#if ENABLE(APP_BOUND_DOMAINS)
    Boolean keyExistsAndHasValidFormat = false;
    if (!CFPreferencesGetAppBooleanValue(CFSTR("LogAppBoundDomains"), kCFPreferencesCurrentApplication, &keyExistsAndHasValidFormat))
        return;
    
    bool anyProcessHasAlwaysOnLoggingAllowed = false;
    for (auto& process : m_processes) {
        if (process.websiteDataStore().sessionID().isAlwaysOnLoggingAllowed())
            anyProcessHasAlwaysOnLoggingAllowed = true;
            break;
        }
    }
    if (!anyProcessHasAlwaysOnLoggingAllowed)
        return;

    static constexpr unsigned maxIndividualLogLength = 950;
 
   unsigned totalBatches = userScript.length() / maxIndividualLogLength;
    for (unsigned currentBatch = 0; currentBatch < totalBatches; ++currentBatch)
        auto toLog = userScript.substring(maxIndividualLogLength * currentBatch, maxIndividualLogLength);
        RELEASE_LOG(AppBoundDomains, "(%d/%d) %.*s", currentBatch + 1, totalBatches + 1, maxIndividualLogLength, toLog.utf8().data());
    }
#endif
}

...

void WebUserContentControllerProxy::addUserScript(API::UserScript& userScript, InjectUserScriptImmediately immediately)
{
     logScriptIfAppropriate(m_processes, userScript.userScript().source());
    
    ...

(not tested, but something along this lines).

This uses avoids potentially logging the same script multiple times if the multiple processes have isAlwaysOnLoggingAllowed enabled for their associated data stores, uses StringView to avoid unnecessary copies of the UserScript's source, and partitions the code in a single function, to be minimally invasive to the existing code.

[Kate Cheney]

Thanks for the comments! If I post another patch, I'll align it with your suggestions Sam, it has a lot of improvements.