AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations #22334

petemill · 2024-02-27T10:11:09Z

Resolves brave/brave-browser#34945

Creates throttle to intercept a specific subresource request
Parses the subresource and shares transcript decision with existing JS global extraction method (which is also used for the initial page in the history)
Creates a new mojom interface for the renderer to ~~send the new content detected back to the browser~~ notify the browser that page-changing content was detected. Actual content sending from renderer will only occur when asked for by browser during a user-initiated AIChat event.
Tests for throttle and for transcript decision making

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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Contains unit and browser tests.

Navigate to youtube.com
Navigate to a video (same-tab)
optional: ask Leo to summarize
Navigate to another video (same-tab)
ask Leo to summarize
Observe that with this PR, the correct video content is summarised and without then only the video content from the initially-navigated-to youtube page is available to Leo.

components/ai_chat/content/browser/page_content_fetcher.h

components/ai_chat/content/browser/ai_chat_tab_helper.cc

petemill · 2024-02-28T09:00:54Z

Creating security review

petemill · 2024-02-29T23:15:50Z

components/ai_chat/renderer/ai_chat_resource_sniffer_url_loader.cc

+
+namespace {
+
+constexpr uint32_t kReadBufferSize = 37000;  // average subresource size


Wasn't 100% sure on what to use for this part...

nullhook · 2024-03-05T19:15:11Z

components/ai_chat/content/browser/ai_chat_tab_helper.cc

+    DVLOG(4) << "Not binding extractor host to non-main frame";
+    return;
+  }
+  auto* sender = content::WebContents::FromRenderFrameHost(rfh);


i know you love auto but i can't infer what the type of sender is here.

Really? It's literally in rhs - content::WebContents::From.... That's why I've used auto here. It's unneccessary repetition in the same line.

nullhook · 2024-03-05T19:15:29Z

components/ai_chat/content/browser/ai_chat_tab_helper.cc

+    DVLOG(1) << "Cannot bind extractor host, no valid WebContents";
+    return;
+  }
+  auto* tab_helper = AIChatTabHelper::FromWebContents(sender);


oh, it's webcontents*

components/ai_chat/renderer/page_content_extractor.cc

nullhook · 2024-03-06T02:39:15Z

components/ai_chat/renderer/page_content_extractor.cc

+  // "page" change.
+  mojo::AssociatedRemote<mojom::PageContentExtractorHost> host;
+  render_frame()->GetRemoteAssociatedInterfaces()->GetInterface(&host);
+  if (host.is_bound()) {


why do you need to check if it's bound? GetInterface gurantees a bind, right? it's internally calling BindNewEndpointAndPassReceiver on a remote.

removed the condition

iefremov · 2024-03-13T10:38:18Z

components/ai_chat/renderer/ai_chat_resource_sniffer_url_loader.h

+namespace ai_chat {
+
+class AIChatResourceSnifferURLLoader
+    : public body_sniffer::BodySnifferURLLoader {


there is an upcoming refactoring of the body sniffer, hopefully the new sniffer design is more logical than the current

sorry i've forgotten to link https://github.com/brave/brave-core/pull/21792/files

iefremov · 2024-03-13T11:18:43Z

components/ai_chat/renderer/yt_util.h

+
+// Parse YT metadata json string and choose the most appropriate caption track
+// url.
+std::optional<std::string> ParseAndChooseCaptionTrackUrl(std::string& body);


i think the parameter should be either const reference or string_view

iefremov · 2024-03-13T11:19:23Z

components/ai_chat/renderer/yt_util.h

+// Extract a caption url from an array of YT caption tracks, from the YT page
+// API.
+std::optional<std::string> ChooseCaptionTrackUrl(
+    base::Value::List* caption_tracks);


boocmp · 2024-03-13T11:25:53Z

components/ai_chat/renderer/ai_chat_resource_sniffer_throttle_unittest.cc

@@ -0,0 +1,319 @@
+// Copyright (c) 2024 The Brave Authors. All rights reserved.


I'm not entirely sure, but it looks like browser tests would be easier to write, also they can test some near to real api calls.

I'm following upstreams example from MimeSniffingThrottle. And these tests seem successful in testing the parts we care about - whether the throttle was created and whether the delegate is called as expected. I think the team prefers unit tests to browser tests, in general - less flakey and more performant. Any issue with this?

bridiver · 2024-03-14T16:42:27Z

components/ai_chat/renderer/ai_chat_resource_sniffer_throttle.cc

+  // |mojom::PageContent|.
+  if (url.SchemeIsHTTPOrHTTPS() && base::Contains(kYouTubeHosts, url.host()) &&
+      base::EqualsCaseInsensitiveASCII(url.path(), kYouTubePlayerAPIPath)) {
+    VLOG(1) << __func__ << " Creating throttle for url: " << url.spec();


this kind of logging should normally be removed before merge per chromium guidelines

NOLOG here, in prod builds +1

petemill · 2024-03-19T05:57:58Z

@iefremov @boocmp perhaps we can merge this before #21792 as this is a P2 issue and will need to be uplifted

thypon · 2024-03-19T10:53:52Z

components/ai_chat/renderer/ai_chat_resource_sniffer_throttle.cc

+  // |mojom::PageContent|.
+  if (url.SchemeIsHTTPOrHTTPS() && base::Contains(kYouTubeHosts, url.host()) &&
+      base::EqualsCaseInsensitiveASCII(url.path(), kYouTubePlayerAPIPath)) {
+    VLOG(1) << __func__ << " Creating throttle for url: " << url.spec();


NOLOG here, in prod builds +1

components/ai_chat/renderer/page_content_extractor.cc

components/ai_chat/renderer/ai_chat_resource_sniffer_url_loader.cc

bcaller

We looked over the PR together

iefremov · 2024-03-20T19:32:58Z

in general the PR looks good to me, pls fix nits. Also the amount of logging probably could be decreased. It seems Pavel is ok to merge it before the body sniffer refactoring

… metadata for same-page navigations

…ai chat message is sent by the user

github-actions · 2024-03-21T04:43:41Z

[puLL-Merge] - brave/brave-core@22334

Description

This PR makes changes to improve the content extraction for AI Chat conversations, particularly for YouTube videos. It introduces a new resource throttle that intercepts specific YouTube API requests and parses out caption track URLs. This allows AI Chat to get up-to-date caption data even when the page content doesn't change via navigation.

Changes

Changes

browser/brave_content_browser_client.cc:

Registers new Mojo interfaces for the AI Chat page content extractor host

chromium_src/chrome/renderer/chrome_content_renderer_client.cc:

Initializes the AI Chat page content extractor in the renderer process if AI Chat is enabled and not in incognito mode

components/ai_chat/content/browser/ai_chat_tab_helper.cc|h:

Adds ability to bind the page content extractor host
Handles intercepted page content change events from the renderer

components/ai_chat/content/browser/page_content_fetcher.cc:

Refactors the PageContentFetcher class to take the URLLoaderFactory in the constructor instead of Start methods

components/ai_chat/core/browser/conversation_driver.cc|h:

Adds OnPageContentUpdated method to handle out-of-band page content updates from the renderer

components/ai_chat/core/common/mojom/page_content_extractor.mojom:

Defines new PageContentExtractorHost interface for renderer->browser communication

components/ai_chat/renderer/*:

Implements the AI Chat resource throttle to intercept YouTube player API requests
Parses the caption track URLs out of the YouTube player API response
Sends the extracted content to the browser process via the PageContentExtractorHost interface

renderer/brave_url_loader_throttle_provider_impl.cc:

Instantiates the AI Chat resource throttle for YouTube player API requests if AI Chat is enabled

Security Hotspots

No major security risks identified. The main additions are:

New Mojo interfaces for renderer->browser communication of extracted page content. These follow standard Chrome practices.
Resource throttle to inspect YouTube API responses. This is limited to specific YouTube URLs to avoid unnecessary overhead. The content extraction doesn't involve untrusted inputs.

The changes look reasonable from a security perspective. As always, parsing of untrusted content like web pages and APIs responses should be done cautiously. The existing unit tests help validate the safety of the parsing logic.

petemill · 2024-03-21T06:23:23Z

@iefremov nits fixed, please take a look. Logging decreased / changed to DVLOG where I can. I'll do a follow-up to change existing logs to DVLOG. They are very useful in general for debugging the endless stream of issues with web content since AI Chat is dependent on that, and a hassle to have to keep removing and adding.

thypon · 2024-03-21T18:37:16Z

good to go @petemill @diracdeltas

… metadata for same-page navigations (#22334) * AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations * optimization: don't parse yt metadata (or fetch transcript) until an ai chat message is sent by the user

kjozwiak · 2024-03-26T02:51:58Z

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

Brave | 1.66.33 Chromium: 123.0.6312.58 (Official Build) nightly (64-bit)
-- | --
Revision | 47f11b3f5c715a0d5d551adb1b4028fd12c8dcca
OS | Windows 11 Version 23H2 (Build 22631.3296)

Using 1.66.24 Chromium: 123.0.6312.58 and the STR/Cases outlined via #22334 (comment), reproduced the issue where Leo wouldn't summarize YT videos correctly when you're summarizing several in a row. For example, in this case, it wouldn't summarize the video and started describing Leo (the feature) rather than using the videos transcripts to summarize what the user is currently watching:

Using the same STR/Cases mentioned above, verified that each YT video was being summarized correctly as per the following:

`Example`	`Example`

Also ensured that the original issue that was described via brave/brave-browser#34945 (comment) wasn't occurring even though I couldn't reproduce the issue using 1.66.24 Chromium: 123.0.6312.58.

… metadata for same-page navigations (#22334) * AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations * optimization: don't parse yt metadata (or fetch transcript) until an ai chat message is sent by the user

…tect new content metadata for same-page navigations (#22744) AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations (#22334) * AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations * optimization: don't parse yt metadata (or fetch transcript) until an ai chat message is sent by the user

…tect new content metadata for same-page navigations (#22745) AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations (#22334) * AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations * optimization: don't parse yt metadata (or fetch transcript) until an ai chat message is sent by the user

petemill requested review from bridiver, yrliou and nullhook February 27, 2024 10:11

petemill self-assigned this Feb 27, 2024

petemill requested a review from a team as a code owner February 27, 2024 10:11

github-actions bot added the puLL-Merge label Feb 27, 2024

petemill commented Feb 27, 2024

View reviewed changes

components/ai_chat/content/browser/page_content_fetcher.h Outdated Show resolved Hide resolved

github-actions bot reviewed Feb 27, 2024

View reviewed changes

components/ai_chat/content/browser/ai_chat_tab_helper.cc Outdated Show resolved Hide resolved

github-actions bot added the needs-security-review label Feb 27, 2024

github-actions bot assigned goodov, iefremov and thypon Feb 27, 2024

petemill commented Feb 27, 2024

View reviewed changes

components/ai_chat/content/browser/ai_chat_tab_helper.cc Outdated Show resolved Hide resolved

petemill force-pushed the ai-chat-subresource-throttle branch 3 times, most recently from 4845e53 to ca507fa Compare February 28, 2024 08:54

petemill commented Feb 29, 2024

View reviewed changes

nullhook reviewed Mar 5, 2024

View reviewed changes

nullhook reviewed Mar 6, 2024

View reviewed changes

iefremov reviewed Mar 13, 2024

View reviewed changes

boocmp reviewed Mar 13, 2024

View reviewed changes

petemill force-pushed the ai-chat-subresource-throttle branch from c416d95 to 0a7a360 Compare March 14, 2024 05:31

bridiver reviewed Mar 14, 2024

View reviewed changes

thypon reviewed Mar 19, 2024

View reviewed changes

bcaller approved these changes Mar 19, 2024

View reviewed changes

petemill added 7 commits March 20, 2024 21:02

AI Chat: sniff subresource content via throttle to detect new content…

77aca7b

… metadata for same-page navigations

optimization: don't parse yt metadata (or fetch transcript) until an …

b55f1ee

…ai chat message is sent by the user

merge fix

f3ed42e

comment

2422104

gn_check fix

089cbfe

rebase fix

f9e1953

amend optimization

536b765

petemill force-pushed the ai-chat-subresource-throttle branch from 6f42779 to 2780596 Compare March 21, 2024 04:42

petemill force-pushed the ai-chat-subresource-throttle branch 2 times, most recently from 55b8b55 to f36d8ff Compare March 21, 2024 05:21

review feedback

2e9a5bf

petemill force-pushed the ai-chat-subresource-throttle branch from f36d8ff to 2e9a5bf Compare March 21, 2024 05:23

iefremov approved these changes Mar 21, 2024

View reviewed changes

petemill merged commit 5f8f514 into master Mar 21, 2024
19 checks passed

petemill deleted the ai-chat-subresource-throttle branch March 21, 2024 21:46

github-actions bot added this to the 1.66.x - Nightly milestone Mar 21, 2024

Uni-verse mentioned this pull request Mar 26, 2024

AI Chat: Fix video transcripts for youtube navigations brave/brave-browser#34945

Closed

ShivanKaul mentioned this pull request Apr 10, 2024

YouTube: XMLHttpRequest send() method freezes tab/page brave/brave-browser#37341

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations #22334

AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations #22334

petemill commented Feb 27, 2024 •

edited

petemill commented Feb 28, 2024

petemill Feb 29, 2024

nullhook Mar 5, 2024

petemill Mar 19, 2024

nullhook Mar 5, 2024

nullhook Mar 6, 2024

petemill Mar 21, 2024

iefremov Mar 13, 2024

iefremov Mar 14, 2024

iefremov Mar 13, 2024

iefremov Mar 13, 2024

boocmp Mar 13, 2024

petemill Mar 13, 2024

bridiver Mar 14, 2024

thypon Mar 19, 2024

petemill commented Mar 19, 2024

thypon Mar 19, 2024

bcaller left a comment

iefremov commented Mar 20, 2024

github-actions bot commented Mar 21, 2024

Changes

petemill commented Mar 21, 2024

thypon commented Mar 21, 2024

kjozwiak commented Mar 26, 2024 •

edited


		namespace {

		constexpr uint32_t kReadBufferSize = 37000; // average subresource size

		@@ -0,0 +1,319 @@
		// Copyright (c) 2024 The Brave Authors. All rights reserved.

AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations #22334

AI Chat: sniff subresource content via throttle to detect new content metadata for same-page navigations #22334

Conversation

petemill commented Feb 27, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

petemill commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Mar 19, 2024

Choose a reason for hiding this comment

bcaller left a comment

Choose a reason for hiding this comment

iefremov commented Mar 20, 2024

github-actions bot commented Mar 21, 2024

Description

Changes

Security Hotspots

petemill commented Mar 21, 2024

thypon commented Mar 21, 2024

kjozwiak commented Mar 26, 2024 • edited

petemill commented Feb 27, 2024 •

edited

kjozwiak commented Mar 26, 2024 •

edited