Bug 222217 – PCM: Introduce PrivateClickMeasurementNetworkLoader

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

[Jiewen Tan]

Figure out a new way to do the network pipeline such that contents of the response can be processed.

[Radar WebKit Bug Importer]

<rdar://problem/74817863>

[Jiewen Tan]

Created attachment 424386 [details]
Patch

[Jiewen Tan]

Created attachment 424388 [details]
Patch

[youenn fablet]

Comment on attachment 424388 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkSession.cpp:124
>      m_privateClickMeasurement = makeUnique<PrivateClickMeasurementManager>(*this, networkProcess, parameters.sessionID);

Since we are creating the PCM here, let's pass the network load function as a parameter to the constructor.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:125
> +    m_privateClickMeasurement->setNetworkLoadFunction([this, weakThis = makeWeakPtr(this)] (NetworkLoadParameters&& loadParameters, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&, const RefPtr<JSON::Object>&)>&& completionHandler) {

Could use auto&& loadParameters, auto&& completionHandler.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:127
>              return;

We probably need to call completionHandler if weakThis is null.
No need to capture 'this' as well.
You could replace with PrivateClickMeasurementNetworkLoader::start(*weakThis,...

> Source/WebKit/NetworkProcess/NetworkSession.h:218
> +    HashSet<std::unique_ptr<PrivateClickMeasurementNetworkLoader>> m_privateClickMeasurementNetworkLoaders;

I think Alex added support for map/set of UniqueRef. You could have a try here.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:155
> +    ASSERT(m_networkLoadFunction);

You could add this one in the constructor if we pass it a construction time.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:156
> +    m_networkLoadFunction(WTFMove(loadParameters), [weakThis = makeWeakPtr(*this), this, attribution = WTFMove(attribution), callback = WTFMove(callback)] (const WebCore::ResourceError& error, const WebCore::ResourceResponse& response, const RefPtr<JSON::Object>& jsonObject) mutable {

Could use more auto.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
> +            m_networkProcess->broadcastConsoleMessage(weakThis->m_sessionID, MessageSource::PrivateClickMeasurement, MessageLevel::Error, makeString("[Private Click Measurement] JSON response is empty for token public key request."_s));

Either stop capture 'this' and use weakThis everywhere or use this.
Do we need the call to makeString?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:41
> +    auto loader = std::unique_ptr<PrivateClickMeasurementNetworkLoader>(new PrivateClickMeasurementNetworkLoader(session, WTFMove(parameters), WTFMove(completionHandler)));

makeUnique?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:79
> +            m_decoder = TextResourceDecoder::create("application/json"_s, "UTF-8");

Could use a one liner maybe:
m_decoder = TextResourceDecoder::create("application/json"_s,  m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:92
> +    if (!jsonValue)

if (auto jsonValue = ...)

> LayoutTests/http/tests/privateClickMeasurement/resources/fraudPreventionTestURL.py:60
> +    'Content-Type: application/json\r\n\r\n'

We do not check Content-Type in PCM loader.
Should we do so? Should we test with and without it?

Do we have coverage for the case of empty application/json body

[Jiewen Tan]

Comment on attachment 424388 [details]
Patch

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

Thanks Youenn for r+ this patch.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:124
>>      m_privateClickMeasurement = makeUnique<PrivateClickMeasurementManager>(*this, networkProcess, parameters.sessionID);
> 
> Since we are creating the PCM here, let's pass the network load function as a parameter to the constructor.

Sounds good to me.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:125
>> +    m_privateClickMeasurement->setNetworkLoadFunction([this, weakThis = makeWeakPtr(this)] (NetworkLoadParameters&& loadParameters, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&, const RefPtr<JSON::Object>&)>&& completionHandler) {
> 
> Could use auto&& loadParameters, auto&& completionHandler.

It sounds like black magic. Fixed.

>> Source/WebKit/NetworkProcess/NetworkSession.cpp:127
>>              return;
> 
> We probably need to call completionHandler if weakThis is null.
> No need to capture 'this' as well.
> You could replace with PrivateClickMeasurementNetworkLoader::start(*weakThis,...

Fixed.

>> Source/WebKit/NetworkProcess/NetworkSession.h:218
>> +    HashSet<std::unique_ptr<PrivateClickMeasurementNetworkLoader>> m_privateClickMeasurementNetworkLoaders;
> 
> I think Alex added support for map/set of UniqueRef. You could have a try here.

I'm not sure though.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:155
>> +    ASSERT(m_networkLoadFunction);
> 
> You could add this one in the constructor if we pass it a construction time.

Fixed.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:156
>> +    m_networkLoadFunction(WTFMove(loadParameters), [weakThis = makeWeakPtr(*this), this, attribution = WTFMove(attribution), callback = WTFMove(callback)] (const WebCore::ResourceError& error, const WebCore::ResourceResponse& response, const RefPtr<JSON::Object>& jsonObject) mutable {
> 
> Could use more auto.

Fixed.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:166
>> +            m_networkProcess->broadcastConsoleMessage(weakThis->m_sessionID, MessageSource::PrivateClickMeasurement, MessageLevel::Error, makeString("[Private Click Measurement] JSON response is empty for token public key request."_s));
> 
> Either stop capture 'this' and use weakThis everywhere or use this.
> Do we need the call to makeString?

Fixed.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:41
>> +    auto loader = std::unique_ptr<PrivateClickMeasurementNetworkLoader>(new PrivateClickMeasurementNetworkLoader(session, WTFMove(parameters), WTFMove(completionHandler)));
> 
> makeUnique?

No, we couldn't. The constructor is private.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:79
>> +            m_decoder = TextResourceDecoder::create("application/json"_s, "UTF-8");
> 
> Could use a one liner maybe:
> m_decoder = TextResourceDecoder::create("application/json"_s,  m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));

Fixed.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:92
>> +    if (!jsonValue)
> 
> if (auto jsonValue = ...)

Fixed.

>> LayoutTests/http/tests/privateClickMeasurement/resources/fraudPreventionTestURL.py:60
>> +    'Content-Type: application/json\r\n\r\n'
> 
> We do not check Content-Type in PCM loader.
> Should we do so? Should we test with and without it?
> 
> Do we have coverage for the case of empty application/json body

Added the check, and the test cases.

[John Wilander]

Comment on attachment 424388 [details]
Patch

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

Looks good over all. Please see my comments.

> Source/WebKit/ChangeLog:9
> +        This patch refactors PrivateClickMeasurementManager to use the newly introduced PrivateClickMeasurementNetworkLoader

I would say "... to use a newly introduced ..." since this patch adds that class.

> Source/WebKit/ChangeLog:10
> +        instead of the PingLoad to handle network traffics such that PCMM can then extract contents of the response.

"... such that PCMM can receive and process response bodies."

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:-125
> -    loadParameters.identifier = ++identifier;

Do we know why NetworkResourceLoadParameters needs an identifier but NetworkLoadParameters doesn't?

> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:64
> +    fail(ResourceError { ResourceError::Type::Cancellation });

Interesting solution.

>>> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:79
>>> +            m_decoder = TextResourceDecoder::create("application/json"_s, "UTF-8");
>> 
>> Could use a one liner maybe:
>> m_decoder = TextResourceDecoder::create("application/json"_s,  m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));
> 
> Fixed.

You should use WebCore::HTTPHeaderValues::applicationJSONContentType() and there you'll see this comment:
    // The default encoding is UTF-8: https://www.ietf.org/rfc/rfc4627.txt.
... which means you don't need to set UTF-8.

[Jiewen Tan]

Comment on attachment 424388 [details]
Patch

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

Thanks John for reviewing the patch.

>> Source/WebKit/ChangeLog:9
>> +        This patch refactors PrivateClickMeasurementManager to use the newly introduced PrivateClickMeasurementNetworkLoader
> 
> I would say "... to use a newly introduced ..." since this patch adds that class.

Fixed.

>> Source/WebKit/ChangeLog:10
>> +        instead of the PingLoad to handle network traffics such that PCMM can then extract contents of the response.
> 
> "... such that PCMM can receive and process response bodies."

Fixed.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:-125
>> -    loadParameters.identifier = ++identifier;
> 
> Do we know why NetworkResourceLoadParameters needs an identifier but NetworkLoadParameters doesn't?

A good question for Youenn.

>>>> Source/WebKit/NetworkProcess/PrivateClickMeasurementNetworkLoader.cpp:79
>>>> +            m_decoder = TextResourceDecoder::create("application/json"_s, "UTF-8");
>>> 
>>> Could use a one liner maybe:
>>> m_decoder = TextResourceDecoder::create("application/json"_s,  m_response.textEncodingName().isEmpty() ? TextEncoding("UTF-8") : TextEncoding(m_response.textEncodingName()));
>> 
>> Fixed.
> 
> You should use WebCore::HTTPHeaderValues::applicationJSONContentType() and there you'll see this comment:
>     // The default encoding is UTF-8: https://www.ietf.org/rfc/rfc4627.txt.
> ... which means you don't need to set UTF-8.

Sure.

[John Wilander]

I filed two follow-up bugs and linked them here.

[Jiewen Tan]

Created attachment 424721 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 424721 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Jiewen Tan]

Created attachment 424724 [details]
Patch for landing

[EWS]

Committed r275260: <https://commits.webkit.org/r275260>

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