Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove P3A protobuf reporting #23147

Closed
rillian opened this issue May 30, 2022 · 9 comments · Fixed by brave/brave-core#13539
Closed

Remove P3A protobuf reporting #23147

rillian opened this issue May 30, 2022 · 9 comments · Fixed by brave/brave-core#13539

Comments

@rillian
Copy link

rillian commented May 30, 2022

Description

Once we've confirmed the json-format P3A reports are behaving properly, and we've adjusted the stats backend to use them over the old protobuf format, we should stop sending the old format.

@rillian rillian added OS/Android Fixes related to Android browser functionality OS/Desktop labels May 30, 2022
@rillian rillian self-assigned this May 30, 2022
rillian added a commit to brave/brave-core that referenced this issue May 30, 2022
Now that we're confident receiving the json-format reports,
stop sending duplicate protobuf-encoded versions.

Resolves brave/brave-browser#23147
@diracdeltas
Copy link
Member

@rillian does this mean that https://github.com/brave/brave-core/blob/920bba00b6efb79aa6a1844c2dcca41eee066442/components/p3a/p3a_message.cc#L22 is no longer being used?

some of that looks unsafe to me but not obviously exploitable... still would be good to remove that function if no longer needed

there is a potential overflow here https://github.com/brave/brave-core/blob/920bba00b6efb79aa6a1844c2dcca41eee066442/components/p3a/p3a_message.cc#L58
like if metastring.size() and metric_value_str.size() are UINT_MAX/2, metastring.size() + metric_value_str.size() is 0 which will bypass that CHECK
but then the memcpy's in https://github.com/brave/brave-core/blob/920bba00b6efb79aa6a1844c2dcca41eee066442/components/p3a/p3a_message.cc#L60-L62 will be out of bounds of data

@rillian
Copy link
Author

rillian commented May 30, 2022

@diracdeltas Unfortunately the code is still needed to send P2A messages until we've also migrated that backend to json reporting.

AFAIR the overflow you pointed out isn't possible with data as constructed; all the literal values are too small. I can add checked arithmetic to guard against values from corrupted state though?

Thanks for the review!

@diracdeltas
Copy link
Member

I can add checked arithmetic to guard against values from corrupted state though?

sgtm!

@GeetaSarvadnya
Copy link

QA blocked until #23754 is resolved

rillian added a commit to brave/brave-core that referenced this issue Jun 30, 2022
With the completion callback issue addressed, skip submission
to the old P3A endpoint. P2A message continue until we've
confirmed adequite numbers on the json endpoint.

Restores brave/brave-browser#23147
@LaurenWags
Copy link
Member

Since this is being reverted by #23754, should this issue now be release-notes/exclude?

wdyt @rillian @rebron @GeetaSarvadnya ?

@Uni-verse
Copy link
Contributor

Uni-verse commented Jul 6, 2022

Verified on Samsung GS 21 running version 12 using 1.41.93, Chromium 103.0.5060.114

Went through STR/Test Plan from brave/brave-core#13539 (comment)

PASS - P3A reports are sent on a fresh profile
PASS - Message all go to p3a-json.brave.com and are formatted as readable JSON.
PASS - Traffic is sent to p3a.brave.com

Encountered - #23922

Screen Shot 2022-07-05 at 6 36 12 PM

@GeetaSarvadnya
Copy link

Since this is being reverted by #23754, should this issue now be release-notes/exclude?

Yes, this should be release note exclude

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Jul 6, 2022

Verification PASSED on

Brave | 1.41.93 Chromium: 103.0.5060.114 (Official Build) (64-bit)
-- | --
Revision | a1c2360c5b02a6d4d6ab33796ad8a268a6128226-refs/branch-heads/5060@{#1124}
OS | Windows 10 Version 21H2 (Build 19044.1766)

Basically we ensured P3A and P2A works as it works in 1.40.x due to brave/brave-core#14015

  • Confirmed that P3A traffic is sent to the endpoint p3a-json.brave.com in 1.41.x
  • Confirmed that all the P3A metrics are sent to the json format to p3a-json.brave.com in 1.41.x
  • Confirmed that after reverting P3A protobuf submission Revert stopping P3A protobuf submission for 1.41 brave-core#14015, the traffic is sent to the old p3a.brave.com endpoint in the encoded format in 1.41.x
  • Confirmed P2A metrics data sent correctly in both old p2a.brave.com and new json p2a.json.brave.com formats in 1.41.x
  • Confirmed brave://local-state file has 6 records for p2a and same is shown in both p2a-json.brave.com and p2a.brave.com endpoints
  • Confirmed brave://local-state file has 47 records for p3a and same is shown in both p3a-json.brave.com and p3a.brave.com endpoints
p3a data in brave://local-state p3a.brave.com p3a-json.brave.com
image image image
p2a data in brave://local-state p2a.brave.com p2a-json.brave.com
image image image

@rillian
Copy link
Author

rillian commented Jul 11, 2022

Since this is being reverted by #23754, should this issue now be release-notes/exclude?

It shouldn't be in the 1.41 release notes, but it's addressed for 1.42 by brave/brave-core#14017 I've updated the milestone but left the release-notes/include tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants