Bug 222217 - PCM: Introduce PrivateClickMeasurementNetworkLoader
Summary: PCM: Introduce PrivateClickMeasurementNetworkLoader
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: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 221291
  Show dependency treegraph
 
Reported: 2021-02-19 18:39 PST by Jiewen Tan
Modified: 2021-03-30 19:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch (34.37 KB, patch)
2021-03-26 11:50 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (34.44 KB, patch)
2021-03-26 12:05 PDT, Jiewen Tan
youennf: review+
Details | Formatted Diff | Diff
Patch for landing (47.32 KB, patch)
2021-03-30 17:41 PDT, Jiewen Tan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (47.40 KB, patch)
2021-03-30 18:24 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2021-02-19 18:39:49 PST
Figure out a new way to do the network pipeline such that contents of the response can be processed.
Comment 1 Radar WebKit Bug Importer 2021-02-26 18:40:19 PST
<rdar://problem/74817863>
Comment 2 Jiewen Tan 2021-03-26 11:50:54 PDT
Created attachment 424386 [details]
Patch
Comment 3 Jiewen Tan 2021-03-26 12:05:45 PDT
Created attachment 424388 [details]
Patch
Comment 4 youenn fablet 2021-03-30 00:39:52 PDT
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
Comment 5 Jiewen Tan 2021-03-30 14:01:22 PDT
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.
Comment 6 John Wilander 2021-03-30 14:11:44 PDT
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.
Comment 7 Jiewen Tan 2021-03-30 14:20:31 PDT
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.
Comment 8 John Wilander 2021-03-30 14:33:42 PDT
I filed two follow-up bugs and linked them here.
Comment 9 Jiewen Tan 2021-03-30 17:41:10 PDT
Created attachment 424721 [details]
Patch for landing
Comment 10 EWS 2021-03-30 17:41:56 PDT
Tools/Scripts/svn-apply failed to apply attachment 424721 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 11 Jiewen Tan 2021-03-30 18:24:23 PDT
Created attachment 424724 [details]
Patch for landing
Comment 12 EWS 2021-03-30 19:52:42 PDT
Committed r275260: <https://commits.webkit.org/r275260>

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