Bug 227731 - PCM: Add error logging for CryptoKit operations
Summary: PCM: Add error logging for CryptoKit operations
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: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-06 18:04 PDT by John Wilander
Modified: 2021-07-07 22:10 PDT (History)
6 users (show)

See Also:

Attachments
Patch (9.22 KB, patch)
2021-07-06 18:25 PDT, John Wilander 		no flags Details | Formatted Diff | Diff
Patch (13.40 KB, patch)
2021-07-07 18:37 PDT, John Wilander 		no flags Details | Formatted Diff | Diff
Patch for landing (13.54 KB, patch)
2021-07-07 21:23 PDT, John Wilander 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2021-07-06 18:04:30 PDT 
There are three FIXMEs for error logging in PCM's CryptoKit operations. We should add the logging.
Comment 1 John Wilander 2021-07-06 18:04:55 PDT 
<rdar://80221057>
Comment 2 John Wilander 2021-07-06 18:25:24 PDT 
Created attachment 432994 [details]
Patch
Comment 3 John Wilander 2021-07-06 18:46:37 PDT 
Comment on attachment 432994 [details]
Patch

Marking as obsolete pending updates to API tests.
Comment 4 John Wilander 2021-07-07 17:15:53 PDT 
A failing API test was addressed in a separate patch (Bugzilla related to this one).
Comment 5 John Wilander 2021-07-07 18:37:14 PDT 
Created attachment 433106 [details]
Patch
Comment 6 Brent Fulgham 2021-07-07 19:08:55 PDT 
Comment on attachment 433106 [details]
Patch

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

r=me, but consider my suggestion about the test case.

> Tools/TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:323
> +    EXPECT_FALSE(pcm.calculateAndUpdateSourceUnlinkableToken(serverPublicKeyBase64URL));

I think these tests would look better if you asserted EXPECT_FALSE(pcm.calculateAndUpdateSourceUnlinkableToken(serverPublicKeyBase64URL).has_value());

The fact that the return value is an error message (or no-value if successful) isn't obvious from the name, so it looks like we are expecting the calculateAndUpdate to fail.
Comment 7 John Wilander 2021-07-07 21:23:19 PDT 
Created attachment 433115 [details]
Patch for landing
Comment 8 John Wilander 2021-07-07 21:24:21 PDT 
(In reply to Brent Fulgham from comment #6)
> Comment on attachment 433106 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433106&action=review
> 
> r=me, but consider my suggestion about the test case.
> 
> > Tools/TestWebKitAPI/Tests/WebCore/PrivateClickMeasurement.cpp:323
> > +    EXPECT_FALSE(pcm.calculateAndUpdateSourceUnlinkableToken(serverPublicKeyBase64URL));
> 
> I think these tests would look better if you asserted
> EXPECT_FALSE(pcm.
> calculateAndUpdateSourceUnlinkableToken(serverPublicKeyBase64URL).
> has_value());
> 
> The fact that the return value is an error message (or no-value if
> successful) isn't obvious from the name, so it looks like we are expecting
> the calculateAndUpdate to fail.

You're absolutely right. I made the test look like the actually call sites with a local variable errorMessage. Should make it clear.

Thanks for the review!
Comment 9 EWS 2021-07-07 22:10:00 PDT 
Committed r279710 (239502@main): <https://commits.webkit.org/239502@main>

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