Add a feature flag to use the Rust Pix QR code validator [chromium/src : main]
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
PTAL. I've updated this to have more metrics and all that fun stuff.
I also log comparative metrics for the C++ validator case, so we can see if there's any interesting deltas from the Rust validator. The CL description still needs a bit of updating, but overall, this should be in a reviewable state finally. Sorry for the delay!
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer voted and added 6 comments![]( "Open in Gerrit")
Votes added by Stephen McGruer
| Code-Review | +1 |
6 comments
Thanks for working on this! LGTM modulo nits and also please update the description.
// Not all possible result variants are mapped to results for metrics.Can you explain the 'why' for this decision in the comment, too? (I assume its just because they will happen too frequently for random copied strings that aren't QR codes at all, so no metric should be logged)
std::optional<PixCodeRustValidationResult> rust_validation_result =Nit; maybe explain in a comment why we always run the rust validator even if the flag is not enabled?
enabled_features.push_back(kEnablePixAccountLinking);Optional nit; could be inlined into line 149, right?
testing::Values(false, true));`testing::Bool`
kNonFinalCrc = 4,Can you remind me what error is thrown if the crc data object is missing entirely?
Related details
- Daniel Cheng
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 7 comments![]( "Open in Gerrit")
// Not all possible result variants are mapped to results for metrics.Can you explain the 'why' for this decision in the comment, too? (I assume its just because they will happen too frequently for random copied strings that aren't QR codes at all, so no metric should be logged)
Done
if (!base::FeatureList::IsEnabled(kUseRustPixCodeValidator) &&
!PixCodeValidator::ContainsPixIdentifier(copied_text_utf8)) {
return;
}Daniel ChengI think we should still continue to check ContainsPixIdentifier as otherwise we'd doing unnecessary work in the PixManager to check the allowlist every time the user copies some text in Chrome.
Our current histogram logs kInvalid for less than 1% of events and would be valuable to continue to log it incase there is some issue in our parsing logic or if the code format changes at some point and that number goes up.
Siddharth Shah`ContainsPixIdentifier()` requires case-folding though, and in the current implementation, requires copying the entire string. The Rust implementation would similarly have to copy the entire string just to casefold for the case-insensitive substring search.
Parsing has many early-rejection points. So I don't actually think there's actually a performance win.
Daniel ChengWhen some text containing a Pix identifier but not a valid Pix code is copied, I agree there might not be significant performance win. However, for text that does not contain the Pix identifier
C++ code: we perform case folding + substring search.
Rust code: we'll be loading the PixManager and doing allowlist check followed by the validator.I think the memory overhead footprint and execution time for the later might be more. WDYT?
Also we'd lose metrics for when a Pix identifier will be lost. If you think that there isn't any performance gain then, I'd propose that we add a PixQrCodeType for Invalid for codes that contain the Pix identifier but fails validation somewhere.
@smcg...@chromium.org please share your thoughts as well. Thanks!
Siddharth ShahOK that reminds me why I structured things the way I did originally: I didn't want to instantiate the PixManager. But the problem with that is that it was hard to log metrics. I was lazy when I addressed @smcg...@chromium.org's comment about metrics and folded all the work back into `PixManager`.
I'll pull the Rust parsing back out separately so we don't instantiate the `PixManager` except for valid codes and add some helpers to log the metrics from outside `PixManager`, but it will be more complicated in the short-term.
I don't think it's useful to say "this thing had a case-insensitive substring that looked like a Pix code but it didn't actually parse as valid". Our parser/validator is very very forgiving.
If we need more metrics, it seems like it'd be far more useful if the validator returned more concrete reasons for failures (e.g. empty additional data or no globally-unique identifier or valid emvco mpm qrcode but not Pix).
Let me ask you this: if we didn't have the data decoder process from the start, would we still have had a `ContainsPixIdentifier()` function? I'm not sure we would have had it at all.
Daniel ChengI don't think it's useful to say "this thing had a case-insensitive substring that looked like a Pix code but it didn't actually parse as valid". Our parser/validator is very very forgiving.
I think this helps if the rate of this metric changes significantly after a Chrome release or a spec update and make us investigate further. I think this is the bare minimum but having more granular metrics like you suggested would be even better.
Let me ask you this: if we didn't have the data decoder process from the start, would we still have had a ContainsPixIdentifier() function? I'm not sure we would have had it at all.
I think so. In the past few months, we've looked at PixFlowExitedReason::kInvalidCode multiple times which has helped product decide on supporting static codes. We also have a metrics funnel that relies on this metric. https://screenshot.googleplex.com/6ih4KpKJWYZxsNF
Siddharth ShahI think so. In the past few months, we've looked at PixFlowExitedReason::kInvalidCode multiple times which has helped product decide on supporting static codes. We also have a metrics funnel that relies on this metric. https://screenshot.googleplex.com/6ih4KpKJWYZxsNF
OK, but I think that means we need better metrics, and not that we specifically need the `kInvalid` metric. For example, the "Pix code but not dynamic" case could have been explicitly detected, and the metric would have been more precise. We can add a distinct enum category for "valid EMVCo merchant-presented mode QRCode that is a Pix code but doesn't validate due to <x|y|z>". Would that address your concerns?
Daniel ChengYes, I think should be sufficient. Thanks!
Acknowledged
std::optional<PixCodeRustValidationResult> rust_validation_result =Nit; maybe explain in a comment why we always run the rust validator even if the flag is not enabled?
Done
// TODO(dcheng): Should this log `kInvalidCode`? This would be quiteDaniel ChengPreviously we'd log this at line 169 for the C++ version. But I feel like it's very spammy.
But that being said, we're still logging `kInvalid` for every single validation attempt... is that something that's still valuable with the Rust validator?
The difference here is:
1. C++ does a very basic test to check if the thing on the clipboard might be a QR code. It early-rejects anything that doesn't pass the basic test.
2. Rust doesn't need to do the very basic test. But that means we report invalid for every single clipboard copy if the merchant is allowlisted.I'm not sure if the allowlisting is going to stay forever–I assume not. So... do we need to log kInvalid at all? If we do, what specific criteria should be used for invalid?
The main reason I didn't implement the early-rejection in Rust is I found it a bit confusing, and it's also impossible to implement efficiently–because there's no non-allocating way to do case-insensitive substring search in either Rust or C++ without implementing something fancy.
Acknowledged
enabled_features.push_back(kEnablePixAccountLinking);Optional nit; could be inlined into line 149, right?
Good point, done.
testing::Values(false, true));Daniel Cheng`testing::Bool`
Oh I forgot about this! Thanks!
Can you remind me what error is thrown if the crc data object is missing entirely?
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
+siashah is OOO for a bit.
+longshen, please let me know if you have any feedback that I should address before landing. Thank you!
Related details
- Longsheng Yin
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
- Longsheng Yin
- Olivia Saul
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivia Saul (Gerrit)
Olivia Saul voted and added 2 comments![]( "Open in Gerrit")
Votes added by Olivia Saul
| Code-Review | +1 |
2 comments
LGTM w/comment for tools/metrics/. Thanks!
<histogram name="FacilitatedPayments.Pix.PaymentCodeValidation.RustResult"I've added a row for this on our go/payments-autofill-uma spreadsheet: [Link](https://docs.google.com/spreadsheets/d/1R5m1KfGLRe3YBvMnjeJ_xeiDQvzTpgx4T0tZioWoqPE/edit?resourcekey=0-TuqaTwhINXPX1vhm5IQ2zA&gid=0#gid=0&range=270:270). Can you please leave comments in the doc for what content should go in columns C and D? I'll copy your comments into the doc itself. Thank you!
Related details
- Daniel Cheng
- Longsheng Yin
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Longsheng Yin (Gerrit)
Longsheng Yin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Longsheng Yin
| Code-Review | +1 |
1 comment
- Daniel Cheng
- Siddharth Shah
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
<histogram name="FacilitatedPayments.Pix.PaymentCodeValidation.RustResult"I've added a row for this on our go/payments-autofill-uma spreadsheet: [Link](https://docs.google.com/spreadsheets/d/1R5m1KfGLRe3YBvMnjeJ_xeiDQvzTpgx4T0tZioWoqPE/edit?resourcekey=0-TuqaTwhINXPX1vhm5IQ2zA&gid=0#gid=0&range=270:270). Can you please leave comments in the doc for what content should go in columns C and D? I'll copy your comments into the doc itself. Thank you!
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Commit-Queue | +2 |
PTAL. I've updated this to have more metrics and all that fun stuff.
I also log comparative metrics for the C++ validator case, so we can see if there's any interesting deltas from the Rust validator. The CL description still needs a bit of updating, but overall, this should be in a reviewable state finally. Sorry for the delay!
Acknowledged
+siashah is OOO for a bit.
+longshen, please let me know if you have any feedback that I should address before landing. Thank you!
Acknowledged
Related details
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
48 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Add a feature flag to use the Rust Pix QR code validator
Currently, the payments driver performs two tests when checking for a Pix
QR code on the clipboard: the first is a fast test that is imprecise but
considered safe to implement in C++, while the second check actually
tries to parse it in C++ in the utility process.
This adds a feature flag to optionally enable the Rust validator: with
the Rust validator, all validation is in-process, so no quick but
imprecise test is needed. To support a gradual transition, this CL adds
a feature flag for using the Rust validator. When the feature flag is
enabled, the payments driver only uses the Rust validator. When the
feature flag is disabled, the payments driver uses the original C++
validator, but also performs an additional Rust validation so that the
results can be compared.
Finally, add additional metrics:
- The Rust validator has a more fine-grained set of results, so add a
new histogram and enum definition when the Rust validator feature is
enabled.
- The C++ validator also logs the result of Rust validation (using an
approximate mapping of the Rust result to the original metrics enum)
to validate that there are no significant differences between the two.
- M components/facilitated_payments/core/browser/BUILD.gn
- M components/facilitated_payments/core/browser/facilitated_payments_driver.cc
- M components/facilitated_payments/core/browser/facilitated_payments_driver_unittest.cc
- M components/facilitated_payments/core/browser/pix_manager.cc
- M components/facilitated_payments/core/browser/pix_manager.h
- M components/facilitated_payments/core/browser/pix_manager_unittest.cc
- M components/facilitated_payments/core/features/features.cc
- M components/facilitated_payments/core/features/features.h
- M components/facilitated_payments/core/metrics/facilitated_payments_metrics.cc
- M components/facilitated_payments/core/metrics/facilitated_payments_metrics.h
- M components/facilitated_payments/core/metrics/facilitated_payments_metrics_unittest.cc
- M components/facilitated_payments/core/validation/BUILD.gn
- M tools/metrics/histograms/metadata/facilitated_payments/enums.xml
- M tools/metrics/histograms/metadata/facilitated_payments/histograms.xml
Code-Review: +1 by Longsheng Yin, +1 by Olivia Saul, +1 by Stephen McGruer
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |