Commit ed4e5169 authored by Yi Gu's avatar Yi Gu Committed by Chromium LUCI CQ

[WebOTP] Record whether the outcome metric is from a cross-origin iframe

For privacy, metrics from inner frames are recorded with the top frame's
origin. Given that WebOTP is supported in cross-origin iframes, it's
better to indicate such information in the |Outcome| metrics to
understand the impact and implications. e.g. does user decline more
often if the API is used in an cross-origin iframe.

UKM collection review:
https://docs.google.com/document/d/12-cSbZ2_ztM4u2USo7cVBlZ5rIiQefkdGxz-XdQfa7Q/edit#

Bug: 1136506, 1127397
Change-Id: I08d9f720ab8b3832bb47d1fb3575ef7093d8fc57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2576588Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Reviewed-by: default avatarKen Buchanan <kenrb@chromium.org>
Reviewed-by: default avatarMajid Valipour <majidvp@chromium.org>
Commit-Queue: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835189}
parent 0ab83176
This diff is collapsed.
...@@ -12,7 +12,8 @@ namespace blink { ...@@ -12,7 +12,8 @@ namespace blink {
void RecordSmsOutcome(WebOTPServiceOutcome outcome, void RecordSmsOutcome(WebOTPServiceOutcome outcome,
ukm::SourceId source_id, ukm::SourceId source_id,
ukm::UkmRecorder* ukm_recorder) { ukm::UkmRecorder* ukm_recorder,
bool is_cross_origin_frame) {
// In |SmsBrowserTest| we wait for UKM to be recorded to avoid race condition // In |SmsBrowserTest| we wait for UKM to be recorded to avoid race condition
// between outcome capture and evaluation. Recording UMA before UKM makes sure // between outcome capture and evaluation. Recording UMA before UKM makes sure
// that |FetchHistogramsFromChildProcesses| reaches the child processes after // that |FetchHistogramsFromChildProcesses| reaches the child processes after
...@@ -23,7 +24,8 @@ void RecordSmsOutcome(WebOTPServiceOutcome outcome, ...@@ -23,7 +24,8 @@ void RecordSmsOutcome(WebOTPServiceOutcome outcome,
DCHECK(ukm_recorder); DCHECK(ukm_recorder);
ukm::builders::SMSReceiver builder(source_id); ukm::builders::SMSReceiver builder(source_id);
builder.SetOutcome(static_cast<int>(outcome)); builder.SetOutcome(static_cast<int>(outcome))
.SetIsCrossOriginFrame(is_cross_origin_frame);
builder.Record(ukm_recorder); builder.Record(ukm_recorder);
} }
......
...@@ -22,7 +22,8 @@ namespace blink { ...@@ -22,7 +22,8 @@ namespace blink {
// iterations of the API. // iterations of the API.
void RecordSmsOutcome(WebOTPServiceOutcome outcome, void RecordSmsOutcome(WebOTPServiceOutcome outcome,
ukm::SourceId source_id, ukm::SourceId source_id,
ukm::UkmRecorder* ukm_recorder); ukm::UkmRecorder* ukm_recorder,
bool is_cross_origin_frame);
// Records the time from when the API is called to when the user successfully // Records the time from when the API is called to when the user successfully
// receives the SMS and presses verify to move on with the verification flow. // receives the SMS and presses verify to move on with the verification flow.
......
...@@ -647,26 +647,40 @@ void OnSmsReceive(ScriptPromiseResolver* resolver, ...@@ -647,26 +647,40 @@ void OnSmsReceive(ScriptPromiseResolver* resolver,
ukm::SourceId source_id = window.UkmSourceID(); ukm::SourceId source_id = window.UkmSourceID();
ukm::UkmRecorder* recorder = window.UkmRecorder(); ukm::UkmRecorder* recorder = window.UkmRecorder();
auto* frame = window.GetFrame();
// For privacy, metrics from inner frames are recorded with the top frame's
// origin. Given that WebOTP is supported in cross-origin iframes, it's better
// to indicate such information in the |Outcome| metrics to understand the
// impact and implications. e.g. does user decline more often if the API is
// used in an cross-origin iframe.
// Based on |IsAncestorChainValidForWebOTP| we want to indicate when the frame
// is cross-origin. i.e. same-origin iframe should not be a concern.
bool is_cross_origin_frame =
!frame->IsMainFrame() && !IsSameOriginWithAncestors(frame);
if (status == mojom::blink::SmsStatus::kUnhandledRequest) { if (status == mojom::blink::SmsStatus::kUnhandledRequest) {
RecordSmsOutcome(WebOTPServiceOutcome::kUnhandledRequest, source_id, RecordSmsOutcome(WebOTPServiceOutcome::kUnhandledRequest, source_id,
recorder); recorder, is_cross_origin_frame);
resolver->Reject(MakeGarbageCollected<DOMException>( resolver->Reject(MakeGarbageCollected<DOMException>(
DOMExceptionCode::kInvalidStateError, DOMExceptionCode::kInvalidStateError,
"OTP retrieval request not handled.")); "OTP retrieval request not handled."));
return; return;
} else if (status == mojom::blink::SmsStatus::kAborted) { } else if (status == mojom::blink::SmsStatus::kAborted) {
RecordSmsOutcome(WebOTPServiceOutcome::kAborted, source_id, recorder); RecordSmsOutcome(WebOTPServiceOutcome::kAborted, source_id, recorder,
is_cross_origin_frame);
resolver->Reject(MakeGarbageCollected<DOMException>( resolver->Reject(MakeGarbageCollected<DOMException>(
DOMExceptionCode::kAbortError, "OTP retrieval was aborted.")); DOMExceptionCode::kAbortError, "OTP retrieval was aborted."));
return; return;
} else if (status == mojom::blink::SmsStatus::kCancelled) { } else if (status == mojom::blink::SmsStatus::kCancelled) {
RecordSmsOutcome(WebOTPServiceOutcome::kCancelled, source_id, recorder); RecordSmsOutcome(WebOTPServiceOutcome::kCancelled, source_id, recorder,
is_cross_origin_frame);
RecordSmsCancelTime(base::TimeTicks::Now() - start_time); RecordSmsCancelTime(base::TimeTicks::Now() - start_time);
resolver->Reject(MakeGarbageCollected<DOMException>( resolver->Reject(MakeGarbageCollected<DOMException>(
DOMExceptionCode::kAbortError, "OTP retrieval was cancelled.")); DOMExceptionCode::kAbortError, "OTP retrieval was cancelled."));
return; return;
} else if (status == mojom::blink::SmsStatus::kTimeout) { } else if (status == mojom::blink::SmsStatus::kTimeout) {
RecordSmsOutcome(WebOTPServiceOutcome::kTimeout, source_id, recorder); RecordSmsOutcome(WebOTPServiceOutcome::kTimeout, source_id, recorder,
is_cross_origin_frame);
// We do not reject the promise as in other branches because the failure // We do not reject the promise as in other branches because the failure
// may not belong to the origin that sends the request. e.g. there are two // may not belong to the origin that sends the request. e.g. there are two
// origins A and B in the queue and A aborts the request. The prompt that // origins A and B in the queue and A aborts the request. The prompt that
...@@ -678,7 +692,8 @@ void OnSmsReceive(ScriptPromiseResolver* resolver, ...@@ -678,7 +692,8 @@ void OnSmsReceive(ScriptPromiseResolver* resolver,
// simultaneously. // simultaneously.
return; return;
} else if (status == mojom::blink::SmsStatus::kUserCancelled) { } else if (status == mojom::blink::SmsStatus::kUserCancelled) {
RecordSmsOutcome(WebOTPServiceOutcome::kUserCancelled, source_id, recorder); RecordSmsOutcome(WebOTPServiceOutcome::kUserCancelled, source_id, recorder,
is_cross_origin_frame);
RecordSmsUserCancelTime(base::TimeTicks::Now() - start_time, source_id, RecordSmsUserCancelTime(base::TimeTicks::Now() - start_time, source_id,
recorder); recorder);
// Similar to kTimeout, the promise is not rejected here. // Similar to kTimeout, the promise is not rejected here.
...@@ -691,13 +706,14 @@ void OnSmsReceive(ScriptPromiseResolver* resolver, ...@@ -691,13 +706,14 @@ void OnSmsReceive(ScriptPromiseResolver* resolver,
// be handled accordingly. e.g. if the user declined the prompt, we record // be handled accordingly. e.g. if the user declined the prompt, we record
// it as |kUserCancelled|. // it as |kUserCancelled|.
RecordSmsOutcome(WebOTPServiceOutcome::kBackendNotAvailable, source_id, RecordSmsOutcome(WebOTPServiceOutcome::kBackendNotAvailable, source_id,
recorder); recorder, is_cross_origin_frame);
// Similar to kTimeout, the promise is not rejected here. // Similar to kTimeout, the promise is not rejected here.
return; return;
} }
RecordSmsSuccessTime(base::TimeTicks::Now() - start_time, source_id, RecordSmsSuccessTime(base::TimeTicks::Now() - start_time, source_id,
recorder); recorder);
RecordSmsOutcome(WebOTPServiceOutcome::kSuccess, source_id, recorder); RecordSmsOutcome(WebOTPServiceOutcome::kSuccess, source_id, recorder,
is_cross_origin_frame);
resolver->Resolve(MakeGarbageCollected<OTPCredential>(otp)); resolver->Resolve(MakeGarbageCollected<OTPCredential>(otp));
} }
......
...@@ -12205,6 +12205,11 @@ be describing additional metrics about the same event. ...@@ -12205,6 +12205,11 @@ be describing additional metrics about the same event.
<summary> <summary>
Records performance metrics for SMSReceiver API. Records performance metrics for SMSReceiver API.
</summary> </summary>
<metric name="IsCrossOriginFrame" enum="Boolean">
<summary>
Flag indicating whether the report comes from a cross-origin frame or not.
</summary>
</metric>
<metric name="Outcome" enum="WebOTPServiceOutcome"> <metric name="Outcome" enum="WebOTPServiceOutcome">
<summary> <summary>
Records the result of a call to the SmsReceiver API. Records the result of a call to the SmsReceiver API.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment