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

Adds back certain UA client hints. #24763

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

szilardszaloki
Copy link
Collaborator

@szilardszaloki szilardszaloki commented Jul 19, 2024

Resolves brave/brave-browser#25694
Security review: https://github.com/brave/reviews/issues/1696

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Load YouTube live streams that have the chat enabled (e.g. https://www.youtube.com/watch?v=_uMuuHk_KkQ) multiple times, and make sure the https://www.youtube.com/live_chat?continuation=... requests never hang (i.e. live chat loads right away/together with the video/page — and not with a 20-sec delay). The 20-sec delay can easily be reprod on any channel/any version at the moment.

@szilardszaloki szilardszaloki marked this pull request as ready for review July 21, 2024 20:44
@szilardszaloki szilardszaloki requested a review from a team as a code owner July 21, 2024 20:44
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix works great! Will defer the implementation details to @ShivanKaul 😄

@szilardszaloki szilardszaloki force-pushed the szilard/25694-fixes-live-chat-on-youtube branch from 76e6634 to 017771a Compare July 25, 2024 18:34
@szilardszaloki szilardszaloki requested a review from a team as a code owner July 25, 2024 18:34
@ShivanKaul ShivanKaul requested a review from mkarolin July 25, 2024 18:46
@ShivanKaul
Copy link
Collaborator

Just a note that we will be clamping to the major version for Sec-CH-UA-Full-Version-List like we did for the JS equivalent in #16177

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@szilardszaloki szilardszaloki merged commit 4473117 into master Jul 25, 2024
16 checks passed
@szilardszaloki szilardszaloki deleted the szilard/25694-fixes-live-chat-on-youtube branch July 25, 2024 22:22
@github-actions github-actions bot added this to the 1.70.x - Nightly milestone Jul 25, 2024
brave-builds added a commit that referenced this pull request Jul 29, 2024
brave-builds added a commit that referenced this pull request Jul 29, 2024
szilardszaloki added a commit that referenced this pull request Jul 30, 2024
szilardszaloki added a commit that referenced this pull request Jul 30, 2024
@kjozwiak
Copy link
Member

kjozwiak commented Aug 6, 2024

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.70.51 Chromium: 127.0.6533.88 (Official Build) nightly (64-bit)
-- | --
Revision | c39ff5a89233f396d214b305233de91ff13a8b9e
OS | Windows 11 Version 23H2 (Build 22631.3958)

Using 1.70.22 Chromium: 127.0.6533.73 and the STR/Cases outlined via #24763 (comment), reproduced the issue where the Live Chat wasn't loading due to https://www.youtube.com/live_chat?continuation=... hanging/being in a Pending state as per the following:

reproducedIssue.mp4

Using the same STR/Cases as per the above, verified that the Live Chat was loading pretty quickly while using 1.70.51 Chromium: 127.0.6533.88 as per the following:

fixedIssueChat.mp4
  • ensured that the video/audio loaded instantly without any issues/ads
  • ensured that the Live Chat loaded instantly without any issues/crashes
  • ensured that https://www.youtube.com/live_chat?continuation=... didn't get stuck in Pending state for ~15-30s.

kjozwiak pushed a commit that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

YouTube live chat remains blank for 30 seconds on live streams
6 participants