Bug 227326 – Clean up App Privacy Report code

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

App Privacy Report code needs some cleanup, mainly renaming and deleting unnecessary code.

[Radar WebKit Bug Importer]

<rdar://problem/79696849>

[Kate Cheney]

Created attachment 432118 [details]
Patch

[Kate Cheney]

Created attachment 432177 [details]
Patch

[Kate Cheney]

Created attachment 432193 [details]
Patch

[Kate Cheney]

Created attachment 432197 [details]
Patch

[Brent Fulgham]

Comment on attachment 432197 [details]
Patch

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

I think this looks good, but I'm concerned there are some places where we have the wrong default set, and I'm not sure if the places where 'AppBound' was used to set 'AppInitiated' is right. r- for those reasons.

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:138
> +    return m_document->loader() ? m_document->loader()->lastNavigationWasAppInitiated() : false;

Shouldn't this default to 'true'?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:572
> +        auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(*certificateInfo), WTFMove(*contentSecurityPolicy), WTFMove(referrerPolicy), WTFMove(scriptURL), *workerType, true, LastNavigationWasAppInitiated::No, WTFMove(scriptResourceMap) };

Should this be LastNavigationWasAppInitiated::Yes, unless we have data indicating it was user initiated?

> Source/WebCore/workers/service/server/SWServer.cpp:658
> +        return LastNavigationWasAppInitiated::No;

Wouldn't the assumption be that it's AppInitiated unless we have data saying it was user initiated? Seems like the default return should be Yes?

> Source/WebCore/workers/service/server/SWServer.cpp:665
> +            return LastNavigationWasAppInitiated::Yes;

Seems like we might want to early return on 'No', since that would be the unusual case.

> Source/WebCore/workers/service/server/SWServer.cpp:668
> +    return LastNavigationWasAppInitiated::No;

And if we early return on No, we would want to return Yes here.

> Source/WebCore/workers/service/server/SWServer.cpp:902
> +    if (data.lastNavigationWasAppInitiated == LastNavigationWasAppInitiated::Yes) {

It seems like we'd almost always be AppInitiated, and would want to relay the User-Initiated nature here, wouldn't we?

> Source/WebCore/workers/service/server/SWServerRegistration.h:138
> +    bool m_isAppInitiated { false };

Shouldn't this default to 'true', like the regular ResourceRequest case?

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:258
> +        registration->scheduleSoftUpdate(m_loader.isAppBound() ? WebCore::IsAppInitiated::Yes : WebCore::IsAppInitiated::No);

Is this conditional right? Why does an App-Bound loader indicate it was app initiated? Should loader have an app Initiated predicate?

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:191
> +            registration->scheduleSoftUpdate(loader.isAppBound() ? WebCore::IsAppInitiated::Yes : WebCore::IsAppInitiated::No);

Ditto.

> Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:372
> +        retrieveSubresourcesEntry(resourceKey, [this, weakThis = makeWeakPtr(*this), frameID, pendingFrameLoad = WTFMove(pendingFrameLoad), requestIsAppBound = request.isAppInitiated(), isNavigatingToAppBoundDomain](std::unique_ptr<SubresourcesEntry> entry) {

Shouldn't 'requestIsAppBound' be renamed, too?

> LayoutTests/platform/ios-wk2/TestExpectations:-1827
> -http/tests/in-app-browser-privacy/app-bound-attribution-post-request.html [ Skip ]

yay!

[Kate Cheney]

Comment on attachment 432197 [details]
Patch

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

Thanks for the review! I'll upload a patch with changes.

>> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:138
>> +    return m_document->loader() ? m_document->loader()->lastNavigationWasAppInitiated() : false;
> 
> Shouldn't this default to 'true'?

Yes, good catch.

>> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:572
>> +        auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(*certificateInfo), WTFMove(*contentSecurityPolicy), WTFMove(referrerPolicy), WTFMove(scriptURL), *workerType, true, LastNavigationWasAppInitiated::No, WTFMove(scriptResourceMap) };
> 
> Should this be LastNavigationWasAppInitiated::Yes, unless we have data indicating it was user initiated?

Ditto, will fix.

>> Source/WebCore/workers/service/server/SWServer.cpp:658
>> +        return LastNavigationWasAppInitiated::No;
> 
> Wouldn't the assumption be that it's AppInitiated unless we have data saying it was user initiated? Seems like the default return should be Yes?

Ditto.

>> Source/WebCore/workers/service/server/SWServer.cpp:665
>> +            return LastNavigationWasAppInitiated::Yes;
> 
> Seems like we might want to early return on 'No', since that would be the unusual case.

Good idea, will change.

>> Source/WebCore/workers/service/server/SWServer.cpp:668
>> +    return LastNavigationWasAppInitiated::No;
> 
> And if we early return on No, we would want to return Yes here.

Ditto.

>> Source/WebCore/workers/service/server/SWServer.cpp:902
>> +    if (data.lastNavigationWasAppInitiated == LastNavigationWasAppInitiated::Yes) {
> 
> It seems like we'd almost always be AppInitiated, and would want to relay the User-Initiated nature here, wouldn't we?

I don't think I understand your suggestion. Currently the heuristic we have for service workers is that if one client for a domain is app-initiated, all clients are considered app-initiated. So here we check if the latest registration client is app initiated and if the current app-initiated value is non app-initiated. If both those things are true then we update the value to be app initiated.

>> Source/WebCore/workers/service/server/SWServerRegistration.h:138
>> +    bool m_isAppInitiated { false };
> 
> Shouldn't this default to 'true', like the regular ResourceRequest case?

Yes, will fix.

>> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:258
>> +        registration->scheduleSoftUpdate(m_loader.isAppBound() ? WebCore::IsAppInitiated::Yes : WebCore::IsAppInitiated::No);
> 
> Is this conditional right? Why does an App-Bound loader indicate it was app initiated? Should loader have an app Initiated predicate?

I think it was a rename that I missed. I will fix it.

>> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:191
>> +            registration->scheduleSoftUpdate(loader.isAppBound() ? WebCore::IsAppInitiated::Yes : WebCore::IsAppInitiated::No);
> 
> Ditto.

Will fix.

>> Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:372
>> +        retrieveSubresourcesEntry(resourceKey, [this, weakThis = makeWeakPtr(*this), frameID, pendingFrameLoad = WTFMove(pendingFrameLoad), requestIsAppBound = request.isAppInitiated(), isNavigatingToAppBoundDomain](std::unique_ptr<SubresourcesEntry> entry) {
> 
> Shouldn't 'requestIsAppBound' be renamed, too?

Yes, will fix.

[Chris Dumez]

Comment on attachment 432197 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServerRegistration.h:110
> +    bool isAppInitiated() { return m_isAppInitiated; }

nit: Should be marked as const.

[Kate Cheney]

Comment on attachment 432197 [details]
Patch

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

>>> Source/WebCore/workers/service/server/SWServer.cpp:665
>>> +            return LastNavigationWasAppInitiated::Yes;
>> 
>> Seems like we might want to early return on 'No', since that would be the unusual case.
> 
> Good idea, will change.

Ok thinking about this more, I actually think we should keep it this way. The heuristic we have for service workers is that if any client was registered as app-initiated for a specific domain, all clients for that domain are app-initiated. This code is enforcing that heuristic, not returning a default value.

>>> Source/WebCore/workers/service/server/SWServer.cpp:668
>>> +    return LastNavigationWasAppInitiated::No;
>> 
>> And if we early return on No, we would want to return Yes here.
> 
> Ditto.

I think we should keep this as-is for the same reason as above. We want to return LastNavigationWasAppInitiated::No if clients exist and none were registered as app-initiated. The early return in this function should cover the case of no clients, which I will flip to be Yes to match the default value.

[Kate Cheney]

Created attachment 433101 [details]
Patch

[Brent Fulgham]

Comment on attachment 432197 [details]
Patch

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

>>>> Source/WebCore/workers/service/server/SWServer.cpp:665
>>>> +            return LastNavigationWasAppInitiated::Yes;
>>> 
>>> Seems like we might want to early return on 'No', since that would be the unusual case.
>> 
>> Good idea, will change.
> 
> Ok thinking about this more, I actually think we should keep it this way. The heuristic we have for service workers is that if any client was registered as app-initiated for a specific domain, all clients for that domain are app-initiated. This code is enforcing that heuristic, not returning a default value.

Ah! Okay -- sounds correct to me.

>>>> Source/WebCore/workers/service/server/SWServer.cpp:668
>>>> +    return LastNavigationWasAppInitiated::No;
>>> 
>>> And if we early return on No, we would want to return Yes here.
>> 
>> Ditto.
> 
> I think we should keep this as-is for the same reason as above. We want to return LastNavigationWasAppInitiated::No if clients exist and none were registered as app-initiated. The early return in this function should cover the case of no clients, which I will flip to be Yes to match the default value.

Agreed.

>>> Source/WebCore/workers/service/server/SWServer.cpp:902
>>> +    if (data.lastNavigationWasAppInitiated == LastNavigationWasAppInitiated::Yes) {
>> 
>> It seems like we'd almost always be AppInitiated, and would want to relay the User-Initiated nature here, wouldn't we?
> 
> I don't think I understand your suggestion. Currently the heuristic we have for service workers is that if one client for a domain is app-initiated, all clients are considered app-initiated. So here we check if the latest registration client is app initiated and if the current app-initiated value is non app-initiated. If both those things are true then we update the value to be app initiated.

Got it -- I didn't understand that, so I think you are correct.

[Brent Fulgham]

Comment on attachment 433101 [details]
Patch

Thank you for cleaning up those suggestions! r=me.

[EWS]

Committed r279750 (239526@main): <https://commits.webkit.org/239526@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433101 [details].