Skip to content

Commit

Permalink
Make CSPP Mojo interfaces associated to a RenderFrame or RenderProcess
Browse files Browse the repository at this point in the history 
We break the PhishingDetector interface into two pieces.
PhishingDetector is scoped to a RenderFrame, and is used to perform
classification on that frame. PhishingModelSetter is scoped to a
RenderProcess, and is used to set the model for a given process.

As part of this change, we create an explicit global scorer object
in the renderer process. This previously existed in
phishing_classifier_delegate.cc, but was in an anonymous namespace, so
many other classes were maintaining references to a Scorer*, which is
no longer necessary.

On the browser side, we used to be pretty aggressive in setting
the model. The ClientSideDetectionService would tell every
ClientSideDetectionHost to change the model in every frame. Since the
model was global to a process, this was a lot of unnecessary Mojo
messages, none of which happen anymore. We also don't need
to track all the ClientSideDetectionHosts or all the
PhishingDetector receivers anymore, since we can simply get the
right receiver for the needed frame.

By sending models at process creation, rather than frame creation, we
also hope to decrease the rate of CLASSIFIER_NOT_READY responses.

Bug: 1217128
Change-Id: I5b2cab1068427903249a0ec36c98dac509a94faf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3615189
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Rohit Bhatia <bhatiarohit@google.com>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001696}
  • Loading branch information
Daniel Rubery authored and Chromium LUCI CQ committed May 10, 2022
1 parent d394a18 commit 7b0479d
Show file tree
Hide file tree
Showing 32 changed files with 383 additions and 415 deletions.
8 changes: 6 additions & 2 deletions chrome/browser/safe_browsing/client_side_detection_host_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class FakeClientSideDetectionService : public ClientSideDetectionService {

void SetModel(const ClientSideModel& model) { model_ = model; }

CSDModelType GetModelType() override { return CSDModelType::kProtobuf; }

const std::string& GetModelStr() override {
client_side_model_ = model_.SerializeAsString();
return client_side_model_;
Expand Down Expand Up @@ -150,9 +152,10 @@ IN_PROC_BROWSER_TEST_F(ClientSideDetectionHostPrerenderBrowserTest,
ChromeClientSideDetectionHostDelegate::CreateHost(
browser()->tab_strip_model()->GetActiveWebContents());
csd_host->set_client_side_detection_service(&fake_csd_service);
csd_host->SendModelToRenderFrame();
csd_host->set_ui_manager(mock_ui_manager.get());

fake_csd_service.SendModelToRenderers();

GURL page_url(embedded_test_server()->GetURL("/safe_browsing/malware.html"));
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), page_url));

Expand Down Expand Up @@ -196,9 +199,10 @@ IN_PROC_BROWSER_TEST_F(ClientSideDetectionHostPrerenderBrowserTest,
ChromeClientSideDetectionHostDelegate::CreateHost(
browser()->tab_strip_model()->GetActiveWebContents());
csd_host->set_client_side_detection_service(&fake_csd_service);
csd_host->SendModelToRenderFrame();
csd_host->set_ui_manager(mock_ui_manager.get());

fake_csd_service.SendModelToRenderers();

base::RunLoop run_loop;
fake_csd_service.SetRequestCallback(run_loop.QuitClosure());

Expand Down
67 changes: 12 additions & 55 deletions chrome/browser/safe_browsing/client_side_detection_host_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,12 @@
#include "content/public/test/test_renderer_host.h"
#include "content/public/test/web_contents_tester.h"
#include "ipc/ipc_test_sink.h"
#include "mojo/public/cpp/bindings/associated_receiver_set.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/receiver_set.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#include "url/gurl.h"

using content::BrowserThread;
Expand Down Expand Up @@ -212,20 +213,10 @@ class FakePhishingDetector : public mojom::PhishingDetector {

~FakePhishingDetector() override = default;

void BindReceiver(mojo::ScopedMessagePipeHandle handle) {
receivers_.Add(this, mojo::PendingReceiver<mojom::PhishingDetector>(
std::move(handle)));
}

// mojom::PhishingDetector
void SetPhishingModel(const std::string& model, base::File file) override {
model_ = model;
}

// mojom::PhishingDetector
void SetPhishingFlatBufferModel(base::ReadOnlySharedMemoryRegion region,
base::File file) override {
region_ = std::move(region);
void BindReceiver(mojo::ScopedInterfaceEndpointHandle handle) {
receivers_.Add(this,
mojo::PendingAssociatedReceiver<mojom::PhishingDetector>(
std::move(handle)));
}

// mojom::PhishingDetector
Expand Down Expand Up @@ -254,25 +245,15 @@ class FakePhishingDetector : public mojom::PhishingDetector {
}
}

void CheckModel(const std::string& model) { EXPECT_EQ(model, model_); }

void CheckModel(base::ReadOnlySharedMemoryRegion region) {
EXPECT_EQ(region.GetGUID(), region_.GetGUID());
}

void Reset() {
phishing_detection_started_ = false;
url_ = GURL();
model_ = "";
region_ = base::ReadOnlySharedMemoryRegion();
}

private:
mojo::ReceiverSet<mojom::PhishingDetector> receivers_;
mojo::AssociatedReceiverSet<mojom::PhishingDetector> receivers_;
bool phishing_detection_started_ = false;
GURL url_;
std::string model_ = "";
base::ReadOnlySharedMemoryRegion region_ = base::ReadOnlySharedMemoryRegion();
};

class ClientSideDetectionHostTestBase : public ChromeRenderViewHostTestHarness {
Expand All @@ -299,12 +280,7 @@ class ClientSideDetectionHostTestBase : public ChromeRenderViewHostTestHarness {
: is_incognito_(is_incognito), model_str_("model_str") {}

void InitTestApi(content::RenderFrameHost* rfh) {
service_manager::InterfaceProvider* remote_interfaces =
rfh->GetRemoteInterfaces();

service_manager::InterfaceProvider::TestApi test_api(remote_interfaces);

test_api.SetBinderForName(
rfh->GetRemoteAssociatedInterfaces()->OverrideBinderForTesting(
mojom::PhishingDetector::Name_,
base::BindRepeating(&FakePhishingDetector::BindReceiver,
base::Unretained(&fake_phishing_detector_)));
Expand Down Expand Up @@ -359,6 +335,10 @@ class ClientSideDetectionHostTestBase : public ChromeRenderViewHostTestHarness {
// SafeBrowsingService.
content::GetUIThreadTaskRunner({})->DeleteSoon(FROM_HERE,
csd_host_.release());

// RenderProcessHostCreationObserver expects to be torn down on UI.
content::GetUIThreadTaskRunner({})->DeleteSoon(FROM_HERE,
csd_service_.release());
database_manager_.reset();
ui_manager_.reset();
base::RunLoop().RunUntilIdle();
Expand Down Expand Up @@ -1201,29 +1181,6 @@ TEST_F(ClientSideDetectionHostTest, RecordsPhishingDetectionDuration) {
.min);
}

TEST_F(ClientSideDetectionHostTest, TestSendFlatBufferModelToRenderFrame) {
base::MappedReadOnlyRegion mapped_region =
base::ReadOnlySharedMemoryRegion::Create(10);
EXPECT_CALL(*csd_service_, GetModelType())
.WillRepeatedly(Return(CSDModelType::kFlatbuffer));
EXPECT_CALL(*csd_service_, GetModelSharedMemoryRegion())
.WillRepeatedly(
Return(testing::ByMove(mapped_region.region.Duplicate())));
csd_host_->SendModelToRenderFrame();
base::RunLoop().RunUntilIdle();
fake_phishing_detector_.CheckModel(mapped_region.region.Duplicate());
fake_phishing_detector_.Reset();
}

TEST_F(ClientSideDetectionHostTest, TestSendModelToRenderFrame) {
std::string stardard("standard");
EXPECT_CALL(*csd_service_, GetModelStr()).WillRepeatedly(ReturnRef(stardard));
csd_host_->SendModelToRenderFrame();
base::RunLoop().RunUntilIdle();
fake_phishing_detector_.CheckModel("standard");
fake_phishing_detector_.Reset();
}

class ClientSideDetectionHostDebugFeaturesTest
: public ClientSideDetectionHostTest {
public:
Expand Down
4 changes: 4 additions & 0 deletions chrome/renderer/chrome_content_renderer_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,8 +424,12 @@ void ChromeContentRendererClient::RenderThreadStarted() {
subresource_filter_ruleset_dealer_ =
std::make_unique<subresource_filter::UnverifiedRulesetDealer>();

phishing_model_setter_ =
std::make_unique<safe_browsing::PhishingModelSetterImpl>();

thread->AddObserver(chrome_observer_.get());
thread->AddObserver(subresource_filter_ruleset_dealer_.get());
thread->AddObserver(phishing_model_setter_.get());

thread->RegisterExtension(extensions_v8::LoadTimesExtension::Get());

Expand Down
3 changes: 3 additions & 0 deletions chrome/renderer/chrome_content_renderer_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "chrome/common/media/webrtc_logging.mojom.h"
#include "chrome/services/speech/buildflags/buildflags.h"
#include "components/nacl/common/buildflags.h"
#include "components/safe_browsing/content/renderer/phishing_classifier/phishing_model_setter_impl.h"
#include "components/spellcheck/spellcheck_buildflags.h"
#include "content/public/renderer/content_renderer_client.h"
#include "content/public/renderer/render_thread.h"
Expand Down Expand Up @@ -274,6 +275,8 @@ class ChromeContentRendererClient
#if BUILDFLAG(ENABLE_PLUGINS)
std::set<std::string> allowed_camera_device_origins_;
#endif
std::unique_ptr<safe_browsing::PhishingModelSetterImpl>
phishing_model_setter_;

scoped_refptr<blink::ThreadSafeBrowserInterfaceBrokerProxy>
browser_interface_broker_;
Expand Down
15 changes: 3 additions & 12 deletions chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,8 @@ class PhishingClassifierTest : public ChromeRenderViewTest,
ASSERT_TRUE(mapped_region_.IsValid());
memcpy(mapped_region_.mapping.memory(), model_str.data(),
model_str.length());
scorer_.reset(FlatBufferModelScorer::Create(
ScorerStorage::GetInstance()->SetScorer(FlatBufferModelScorer::Create(
mapped_region_.region.Duplicate(), base::File()));
ASSERT_TRUE(scorer_.get());
}

void PrepareModel() {
Expand Down Expand Up @@ -197,19 +196,12 @@ class PhishingClassifierTest : public ChromeRenderViewTest,
model.set_max_shingles_per_page(100);
model.set_shingle_size(3);

scorer_.reset(
ScorerStorage::GetInstance()->SetScorer(
ProtobufModelScorer::Create(model.SerializeAsString(), base::File()));
ASSERT_TRUE(scorer_.get());
}

void SetUpClassifier() {
classifier_ = std::make_unique<PhishingClassifier>(GetMainRenderFrame());
// No scorer yet, so the classifier is not ready.
ASSERT_FALSE(classifier_->is_ready());

// Now set the scorer.
classifier_->set_phishing_scorer(scorer_.get());
ASSERT_TRUE(classifier_->is_ready());
}

// Helper method to start phishing classification.
Expand Down Expand Up @@ -247,7 +239,6 @@ class PhishingClassifierTest : public ChromeRenderViewTest,
}

std::string response_content_;
std::unique_ptr<Scorer> scorer_;
std::unique_ptr<PhishingClassifier> classifier_;
base::RunLoop run_loop_;
base::MappedReadOnlyRegion mapped_region_;
Expand Down Expand Up @@ -358,7 +349,7 @@ TEST_P(PhishingClassifierTest, TestClassificationWhenSchemeNotSupported) {
TEST_P(PhishingClassifierTest, DisableDetection) {
EXPECT_TRUE(classifier_->is_ready());
// Set a NULL scorer, which turns detection back off.
classifier_->set_phishing_scorer(NULL);
ScorerStorage::GetInstance()->SetScorer(nullptr);
EXPECT_FALSE(classifier_->is_ready());
}

Expand Down
50 changes: 29 additions & 21 deletions chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
#include "chrome/test/base/chrome_unit_test_suite.h"
#include "components/safe_browsing/content/common/safe_browsing.mojom-shared.h"
#include "components/safe_browsing/content/renderer/phishing_classifier/features.h"
#include "components/safe_browsing/content/renderer/phishing_classifier/flatbuffer_scorer.h"
#include "components/safe_browsing/content/renderer/phishing_classifier/phishing_classifier.h"
#include "components/safe_browsing/content/renderer/phishing_classifier/protobuf_scorer.h"
#include "components/safe_browsing/content/renderer/phishing_classifier/scorer.h"
#include "components/safe_browsing/core/common/fbs/client_model_generated.h"
#include "components/safe_browsing/core/common/proto/csd.pb.h"
Expand Down Expand Up @@ -154,6 +156,8 @@ class PhishingClassifierDelegateTest : public ChromeRenderViewTest {

content::RenderFrame* render_frame = GetMainRenderFrame();
classifier_ = new StrictMock<MockPhishingClassifier>(render_frame);
render_frame->GetAssociatedInterfaceRegistry()->RemoveInterface(
mojom::PhishingDetector::Name_);
delegate_ = PhishingClassifierDelegate::Create(render_frame, classifier_);
}

Expand Down Expand Up @@ -195,8 +199,8 @@ class PhishingClassifierDelegateTest : public ChromeRenderViewTest {
};

TEST_F(PhishingClassifierDelegateTest, Navigation) {
MockScorer scorer;
delegate_->SetPhishingScorer(&scorer);
auto scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
ASSERT_TRUE(classifier_->is_ready());

// Test an initial load. We expect classification to happen normally.
Expand Down Expand Up @@ -318,7 +322,7 @@ TEST_F(PhishingClassifierDelegateTest, Navigation) {

TEST_F(PhishingClassifierDelegateTest, NoPhishingModel) {
ASSERT_FALSE(classifier_->is_ready());
delegate_->SetPhishingModel("", base::File());
ScorerStorage::GetInstance()->SetScorer(nullptr);
// The scorer is nullptr so the classifier should still not be ready.
ASSERT_FALSE(classifier_->is_ready());
}
Expand All @@ -328,7 +332,8 @@ TEST_F(PhishingClassifierDelegateTest, HasPhishingModel) {

ClientSideModel model;
model.set_max_words_per_term(1);
delegate_->SetPhishingModel(model.SerializeAsString(), base::File());
ScorerStorage::GetInstance()->SetScorer(
ProtobufModelScorer::Create(model.SerializeAsString(), base::File()));
ASSERT_TRUE(classifier_->is_ready());

// The delegate will cancel pending classification on destruction.
Expand All @@ -343,8 +348,8 @@ TEST_F(PhishingClassifierDelegateTest, HasFlatBufferModel) {
base::ReadOnlySharedMemoryRegion::Create(model_str.length());
memcpy(mapped_region.mapping.memory(), model_str.data(), model_str.length());

delegate_->SetPhishingFlatBufferModel(mapped_region.region.Duplicate(),
base::File());
ScorerStorage::GetInstance()->SetScorer(FlatBufferModelScorer::Create(
mapped_region.region.Duplicate(), base::File()));
ASSERT_TRUE(classifier_->is_ready());

// The delegate will cancel pending classification on destruction.
Expand All @@ -366,7 +371,8 @@ TEST_F(PhishingClassifierDelegateTest, HasVisualTfLiteModel) {

ClientSideModel model;
model.set_max_words_per_term(1);
delegate_->SetPhishingModel(model.SerializeAsString(), std::move(file));
ScorerStorage::GetInstance()->SetScorer(
ProtobufModelScorer::Create(model.SerializeAsString(), std::move(file)));
ASSERT_TRUE(classifier_->is_ready());

// The delegate will cancel pending classification on destruction.
Expand All @@ -393,8 +399,8 @@ TEST_F(PhishingClassifierDelegateTest, NoScorer) {
// Now set a scorer, which should cause a classifier to be created,
// but no classification will start.
page_text = u"dummy";
MockScorer scorer;
delegate_->SetPhishingScorer(&scorer);
auto scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
Mock::VerifyAndClearExpectations(classifier_);

// Manually start a classification.
Expand All @@ -404,7 +410,8 @@ TEST_F(PhishingClassifierDelegateTest, NoScorer) {
// If we set a new scorer while a classification is going on the
// classification should be cancelled.
EXPECT_CALL(*classifier_, CancelPendingClassification());
delegate_->SetPhishingScorer(&scorer);
scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
Mock::VerifyAndClearExpectations(classifier_);

// The delegate will cancel pending classification on destruction.
Expand All @@ -430,8 +437,8 @@ TEST_F(PhishingClassifierDelegateTest, NoScorer_Ref) {
// Now set a scorer, which should cause a classifier to be created,
// but no classification will start.
page_text = u"dummy";
MockScorer scorer;
delegate_->SetPhishingScorer(&scorer);
auto scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
Mock::VerifyAndClearExpectations(classifier_);

// Manually start a classification.
Expand All @@ -441,7 +448,8 @@ TEST_F(PhishingClassifierDelegateTest, NoScorer_Ref) {
// If we set a new scorer while a classification is going on the
// classification should be cancelled.
EXPECT_CALL(*classifier_, CancelPendingClassification());
delegate_->SetPhishingScorer(&scorer);
scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
Mock::VerifyAndClearExpectations(classifier_);

// The delegate will cancel pending classification on destruction.
Expand All @@ -451,8 +459,8 @@ TEST_F(PhishingClassifierDelegateTest, NoScorer_Ref) {
TEST_F(PhishingClassifierDelegateTest, NoStartPhishingDetection) {
// Tests the behavior when OnStartPhishingDetection has not yet been called
// when the page load finishes.
MockScorer scorer;
delegate_->SetPhishingScorer(&scorer);
auto scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
ASSERT_TRUE(classifier_->is_ready());

EXPECT_CALL(*classifier_, CancelPendingClassification());
Expand Down Expand Up @@ -525,8 +533,8 @@ TEST_F(PhishingClassifierDelegateTest, NoStartPhishingDetection) {

TEST_F(PhishingClassifierDelegateTest, IgnorePreliminaryCapture) {
// Tests that preliminary PageCaptured notifications are ignored.
MockScorer scorer;
delegate_->SetPhishingScorer(&scorer);
auto scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
ASSERT_TRUE(classifier_->is_ready());

EXPECT_CALL(*classifier_, CancelPendingClassification());
Expand Down Expand Up @@ -555,8 +563,8 @@ TEST_F(PhishingClassifierDelegateTest, IgnorePreliminaryCapture) {
TEST_F(PhishingClassifierDelegateTest, DuplicatePageCapture) {
// Tests that a second PageCaptured notification causes classification to
// be cancelled.
MockScorer scorer;
delegate_->SetPhishingScorer(&scorer);
auto scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
ASSERT_TRUE(classifier_->is_ready());

EXPECT_CALL(*classifier_, CancelPendingClassification());
Expand Down Expand Up @@ -586,8 +594,8 @@ TEST_F(PhishingClassifierDelegateTest, DuplicatePageCapture) {
TEST_F(PhishingClassifierDelegateTest, PhishingDetectionDone) {
// Tests that a SafeBrowsingHostMsg_PhishingDetectionDone IPC is
// sent to the browser whenever we finish classification.
MockScorer scorer;
delegate_->SetPhishingScorer(&scorer);
auto scorer = std::make_unique<MockScorer>();
ScorerStorage::GetInstance()->SetScorer(std::move(scorer));
ASSERT_TRUE(classifier_->is_ready());

// Start by loading a page to populate the delegate's state.
Expand Down
1 change: 1 addition & 0 deletions components/safe_browsing/DEPS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ include_rules = [
"+third_party/tflite_support",
"+third_party/tflite",
"+third_party/protobuf",
"+third_party/blink/public/common/associated_interfaces",
"+ui/base/resource/resource_bundle.h",
"+ui/android/view_android.h",

Expand Down

0 comments on commit 7b0479d

Please sign in to comment.