Commit 887fdd82 authored by Dominique Fauteux-Chapleau's avatar Dominique Fauteux-Chapleau Committed by Chromium LUCI CQ

Reland "Handle sync paste deep scanning crash"

This relands crrev.com/c/2633701 that was reverted by
crrev.com/c/2637406. The cause of the flakiness was that the
WebContents corresponding to the scan would get destroyed before the
scan had finished, resulting in a segfault with |web_contents_|. The
solution is to break the dependency of ContentAnalysisDelegate on that
WebContents by extracting the required data from it at a moment when
it's known to be non-null in the ctor.

Original change's description:
> Handle sync paste deep scanning crash
>
> This fixes the crash observed in crbug.com/1166233 and adds tests for
> it. See the comments in that bug for more details.
>
> This fix also removes the dialog for unauthorized users for pasting
> after the initial scan that does authentication, so crbug.com/1028133
> is affected.
>
> Bug: 1166233, 1028133
> Change-Id: Id3c992fdeccfa92a9f88e25c13e7a15bcb1afd8c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2633701
> Reviewed-by: anthonyvd <anthonyvd@chromium.org>
> Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#844787}

Bug: 1166233, 1028133
Change-Id: Ifc760b9489d810c460889e396430c82b4f80e858
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2638533Reviewed-by: default avataranthonyvd <anthonyvd@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845263}
parent f8fe422e
...@@ -188,9 +188,7 @@ void ContentAnalysisDelegate::BypassWarnings() { ...@@ -188,9 +188,7 @@ void ContentAnalysisDelegate::BypassWarnings() {
content_size += (entry.size() * sizeof(base::char16)); content_size += (entry.size() * sizeof(base::char16));
ReportAnalysisConnectorWarningBypass( ReportAnalysisConnectorWarningBypass(
Profile::FromBrowserContext(web_contents_->GetBrowserContext()), profile_, url_, "Text data", std::string(), "text/plain",
web_contents_->GetLastCommittedURL(), "Text data", std::string(),
"text/plain",
extensions::SafeBrowsingPrivateEventRouter::kTriggerWebContentUpload, extensions::SafeBrowsingPrivateEventRouter::kTriggerWebContentUpload,
access_point_, content_size, text_response_); access_point_, content_size, text_response_);
} }
...@@ -201,8 +199,7 @@ void ContentAnalysisDelegate::BypassWarnings() { ...@@ -201,8 +199,7 @@ void ContentAnalysisDelegate::BypassWarnings() {
result_.paths_results[index] = true; result_.paths_results[index] = true;
ReportAnalysisConnectorWarningBypass( ReportAnalysisConnectorWarningBypass(
Profile::FromBrowserContext(web_contents_->GetBrowserContext()), profile_, url_, data_.paths[index].AsUTF8Unsafe(),
web_contents_->GetLastCommittedURL(), data_.paths[index].AsUTF8Unsafe(),
file_info_[index].sha256, file_info_[index].mime_type, file_info_[index].sha256, file_info_[index].mime_type,
extensions::SafeBrowsingPrivateEventRouter::kTriggerFileUpload, extensions::SafeBrowsingPrivateEventRouter::kTriggerFileUpload,
access_point_, file_info_[index].size, warning.second); access_point_, file_info_[index].size, warning.second);
...@@ -359,11 +356,12 @@ ContentAnalysisDelegate::ContentAnalysisDelegate( ...@@ -359,11 +356,12 @@ ContentAnalysisDelegate::ContentAnalysisDelegate(
Data data, Data data,
CompletionCallback callback, CompletionCallback callback,
safe_browsing::DeepScanAccessPoint access_point) safe_browsing::DeepScanAccessPoint access_point)
: web_contents_(web_contents), : data_(std::move(data)),
data_(std::move(data)),
callback_(std::move(callback)), callback_(std::move(callback)),
access_point_(access_point) { access_point_(access_point) {
DCHECK(web_contents_); DCHECK(web_contents);
profile_ = Profile::FromBrowserContext(web_contents->GetBrowserContext());
url_ = web_contents->GetLastCommittedURL();
result_.text_results.resize(data_.text.size(), false); result_.text_results.resize(data_.text.size(), false);
result_.paths_results.resize(data_.paths.size(), false); result_.paths_results.resize(data_.paths.size(), false);
file_info_.resize(data_.paths.size()); file_info_.resize(data_.paths.size());
...@@ -390,9 +388,7 @@ void ContentAnalysisDelegate::StringRequestCallback( ...@@ -390,9 +388,7 @@ void ContentAnalysisDelegate::StringRequestCallback(
text_complies); text_complies);
MaybeReportDeepScanningVerdict( MaybeReportDeepScanningVerdict(
Profile::FromBrowserContext(web_contents_->GetBrowserContext()), profile_, url_, "Text data", std::string(), "text/plain",
web_contents_->GetLastCommittedURL(), "Text data", std::string(),
"text/plain",
extensions::SafeBrowsingPrivateEventRouter::kTriggerWebContentUpload, extensions::SafeBrowsingPrivateEventRouter::kTriggerWebContentUpload,
access_point_, content_size, result, response, access_point_, content_size, result, response,
CalculateEventResult(data_.settings, text_complies, should_warn)); CalculateEventResult(data_.settings, text_complies, should_warn));
...@@ -425,9 +421,7 @@ void ContentAnalysisDelegate::CompleteFileRequestCallback( ...@@ -425,9 +421,7 @@ void ContentAnalysisDelegate::CompleteFileRequestCallback(
result_.paths_results[index] = file_complies; result_.paths_results[index] = file_complies;
MaybeReportDeepScanningVerdict( MaybeReportDeepScanningVerdict(
Profile::FromBrowserContext(web_contents_->GetBrowserContext()), profile_, url_, path.AsUTF8Unsafe(), file_info_[index].sha256, mime_type,
web_contents_->GetLastCommittedURL(), path.AsUTF8Unsafe(),
file_info_[index].sha256, mime_type,
extensions::SafeBrowsingPrivateEventRouter::kTriggerFileUpload, extensions::SafeBrowsingPrivateEventRouter::kTriggerFileUpload,
access_point_, file_info_[index].size, result, response, access_point_, file_info_[index].size, result, response,
CalculateEventResult(data_.settings, file_complies, should_warn)); CalculateEventResult(data_.settings, file_complies, should_warn));
...@@ -479,6 +473,10 @@ bool ContentAnalysisDelegate::UploadData() { ...@@ -479,6 +473,10 @@ bool ContentAnalysisDelegate::UploadData() {
for (const base::FilePath& path : data_.paths) for (const base::FilePath& path : data_.paths)
PrepareFileRequest(path); PrepareFileRequest(path);
data_uploaded_ = true;
// Do not add code under this comment. The above line should be the last thing
// this function does before the return statement.
return !text_request_complete_ || file_result_count_ != data_.paths.size(); return !text_request_complete_ || file_result_count_ != data_.paths.size();
} }
...@@ -529,12 +527,9 @@ void ContentAnalysisDelegate::PrepareFileRequest(const base::FilePath& path) { ...@@ -529,12 +527,9 @@ void ContentAnalysisDelegate::PrepareFileRequest(const base::FilePath& path) {
void ContentAnalysisDelegate::PrepareRequest( void ContentAnalysisDelegate::PrepareRequest(
enterprise_connectors::AnalysisConnector connector, enterprise_connectors::AnalysisConnector connector,
BinaryUploadService::Request* request) { BinaryUploadService::Request* request) {
Profile* profile =
Profile::FromBrowserContext(web_contents_->GetBrowserContext());
request->set_device_token(data_.settings.dm_token); request->set_device_token(data_.settings.dm_token);
request->set_analysis_connector(connector); request->set_analysis_connector(connector);
request->set_email(safe_browsing::GetProfileEmail(profile)); request->set_email(safe_browsing::GetProfileEmail(profile_));
request->set_url(data_.url.spec()); request->set_url(data_.url.spec());
request->set_tab_url(data_.url); request->set_tab_url(data_.url);
for (const std::string& tag : data_.settings.tags) for (const std::string& tag : data_.settings.tags)
...@@ -547,8 +542,7 @@ void ContentAnalysisDelegate::FillAllResultsWith(bool status) { ...@@ -547,8 +542,7 @@ void ContentAnalysisDelegate::FillAllResultsWith(bool status) {
} }
BinaryUploadService* ContentAnalysisDelegate::GetBinaryUploadService() { BinaryUploadService* ContentAnalysisDelegate::GetBinaryUploadService() {
return safe_browsing::BinaryUploadServiceFactory::GetForProfile( return safe_browsing::BinaryUploadServiceFactory::GetForProfile(profile_);
Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
} }
void ContentAnalysisDelegate::UploadTextForDeepScanning( void ContentAnalysisDelegate::UploadTextForDeepScanning(
...@@ -584,8 +578,9 @@ void ContentAnalysisDelegate::MaybeCompleteScanRequest() { ...@@ -584,8 +578,9 @@ void ContentAnalysisDelegate::MaybeCompleteScanRequest() {
if (final_result_ != FinalResult::WARNING) if (final_result_ != FinalResult::WARNING)
RunCallback(); RunCallback();
if (!UpdateDialog()) { if (!UpdateDialog() && data_uploaded_) {
// No UI was shown. Delete |this| to cleanup. // No UI was shown. Delete |this| to cleanup, unless UploadData isn't done
// yet.
delete this; delete this;
} }
} }
......
...@@ -244,7 +244,8 @@ class ContentAnalysisDelegate { ...@@ -244,7 +244,8 @@ class ContentAnalysisDelegate {
private: private:
// Uploads data for deep scanning. Returns true if uploading is occurring in // Uploads data for deep scanning. Returns true if uploading is occurring in
// the background and false if there is nothing to do. // the background and false if there is nothing to do. Sets |data_uploaded_|
// to true right before returning.
bool UploadData(); bool UploadData();
// Prepares an upload request for the text in |data_|. If |data_.text| is // Prepares an upload request for the text in |data_|. If |data_.text| is
...@@ -314,8 +315,11 @@ class ContentAnalysisDelegate { ...@@ -314,8 +315,11 @@ class ContentAnalysisDelegate {
// Virtual to override in tests. // Virtual to override in tests.
virtual safe_browsing::BinaryUploadService* GetBinaryUploadService(); virtual safe_browsing::BinaryUploadService* GetBinaryUploadService();
// The web contents that is attempting to access the data. // The Profile corresponding to the pending scan request(s).
content::WebContents* web_contents_ = nullptr; Profile* profile_ = nullptr;
// The GURL corresponding to the page where the scan triggered.
GURL url_;
// Description of the data being scanned and the results of the scan. // Description of the data being scanned and the results of the scan.
// The elements of the vector |file_info_| hold the FileInfo of the file at // The elements of the vector |file_info_| hold the FileInfo of the file at
...@@ -353,6 +357,10 @@ class ContentAnalysisDelegate { ...@@ -353,6 +357,10 @@ class ContentAnalysisDelegate {
// Scanning result to be shown to the user once every request is done. // Scanning result to be shown to the user once every request is done.
FinalResult final_result_ = FinalResult::SUCCESS; FinalResult final_result_ = FinalResult::SUCCESS;
// Set to true at the end of UploadData to indicate requests have been made
// for every file/text. This is read to ensure |this| isn't deleted too early.
bool data_uploaded_ = false;
base::TimeTicks upload_start_time_; base::TimeTicks upload_start_time_;
base::WeakPtrFactory<ContentAnalysisDelegate> weak_ptr_factory_{this}; base::WeakPtrFactory<ContentAnalysisDelegate> weak_ptr_factory_{this};
......
...@@ -833,4 +833,159 @@ IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegateBlockingSettingBrowserTest, ...@@ -833,4 +833,159 @@ IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegateBlockingSettingBrowserTest,
ASSERT_EQ(FakeBinaryUploadServiceStorage()->requests_count(), 3); ASSERT_EQ(FakeBinaryUploadServiceStorage()->requests_count(), 3);
} }
// This class tests that ContentAnalysisDelegate is handled correctly when the
// requests are already unauthorized. The test parameter represents if the scan
// is set to be blocking through policy.
class ContentAnalysisDelegateUnauthorizedBrowserTest
: public ContentAnalysisDelegateBrowserTest,
public testing::WithParamInterface<bool> {
public:
ContentAnalysisDelegateUnauthorizedBrowserTest() = default;
bool blocking_scan() const { return GetParam(); }
void SetUpScanning(bool file_scan) {
SetDMTokenForTesting(policy::DMToken::CreateValidTokenForTesting(kDmToken));
std::string pref = base::StringPrintf(
R"({
"service_provider": "google",
"enable": [
{
"url_list": ["*"],
"tags": ["dlp", "malware"]
}
],
"block_until_verdict": %d
})",
blocking_scan() ? 1 : 0);
safe_browsing::SetAnalysisConnector(
browser()->profile()->GetPrefs(),
file_scan ? FILE_ATTACHED : BULK_DATA_ENTRY, pref);
file_scan_ = file_scan;
}
// The dialog should only appear on file scans since they need to be opened
// asynchronously. This means that the dialog appears when scanning files and
// that all the following overrides should be called. Text scans don't have an
// asynchronous operation needed before being sent for scanning, so in the
// unauthorized case the dialog doesn't need to appear and the assertions will
// fail the test if they are reached.
void ConstructorCalled(ContentAnalysisDialog* dialog,
base::TimeTicks timestamp) override {
ASSERT_TRUE(file_scan_ && blocking_scan());
}
void ViewsFirstShown(ContentAnalysisDialog* dialog,
base::TimeTicks timestamp) override {
ASSERT_TRUE(file_scan_ && blocking_scan());
}
void DialogUpdated(ContentAnalysisDialog* dialog,
ContentAnalysisDelegate::FinalResult result) override {
ASSERT_TRUE(file_scan_ && blocking_scan());
}
void DestructorCalled(ContentAnalysisDialog* dialog) override {
ASSERT_TRUE(file_scan_ && blocking_scan());
CallQuitClosure();
}
protected:
bool file_scan_ = false;
};
INSTANTIATE_TEST_SUITE_P(,
ContentAnalysisDelegateUnauthorizedBrowserTest,
testing::Bool());
IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegateUnauthorizedBrowserTest, Paste) {
SetUpScanning(/*file_scan*/ false);
ContentAnalysisDelegate::SetFactoryForTesting(
base::BindRepeating(&MinimalFakeContentAnalysisDelegate::Create));
FakeBinaryUploadServiceStorage()->SetAuthForTesting(kDmToken, false);
bool called = false;
base::RunLoop run_loop;
base::RepeatingClosure quit_closure = run_loop.QuitClosure();
ContentAnalysisDelegate::Data data;
data.text.emplace_back(text());
ASSERT_TRUE(ContentAnalysisDelegate::IsEnabled(
browser()->profile(), GURL(kTestUrl), &data, BULK_DATA_ENTRY));
ContentAnalysisDelegate::CreateForWebContents(
browser()->tab_strip_model()->GetActiveWebContents(), std::move(data),
base::BindLambdaForTesting(
[&quit_closure, &called](
const ContentAnalysisDelegate::Data& data,
const ContentAnalysisDelegate::Result& result) {
ASSERT_EQ(result.text_results.size(), 1u);
ASSERT_EQ(result.paths_results.size(), 0u);
ASSERT_TRUE(result.text_results[0]);
called = true;
quit_closure.Run();
}),
safe_browsing::DeepScanAccessPoint::PASTE);
run_loop.Run();
EXPECT_TRUE(called);
// No requests should be made since the DM token is unauthorized.
ASSERT_EQ(FakeBinaryUploadServiceStorage()->requests_count(), 0);
}
IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegateUnauthorizedBrowserTest, Files) {
base::ScopedAllowBlockingForTesting allow_blocking;
SetUpScanning(/*file_scan*/ true);
ContentAnalysisDelegate::SetFactoryForTesting(
base::BindRepeating(&MinimalFakeContentAnalysisDelegate::Create));
FakeBinaryUploadServiceStorage()->SetAuthForTesting(kDmToken, false);
bool called = false;
base::RunLoop run_loop;
base::Optional<base::RepeatingClosure> quit_closure = base::nullopt;
// If the scan is blocking, we can call the quit closure when the dialog
// closes. If it's not, call it at the end of the result callback.
if (blocking_scan())
SetQuitClosure(run_loop.QuitClosure());
else
quit_closure = run_loop.QuitClosure();
ContentAnalysisDelegate::Data data;
CreateFilesForTest({"file1.doc", "file2.doc"}, {"content1", "content2"},
&data);
ASSERT_TRUE(ContentAnalysisDelegate::IsEnabled(
browser()->profile(), GURL(kTestUrl), &data, FILE_ATTACHED));
ContentAnalysisDelegate::CreateForWebContents(
browser()->tab_strip_model()->GetActiveWebContents(), std::move(data),
base::BindLambdaForTesting(
[&quit_closure, &called](
const ContentAnalysisDelegate::Data& data,
const ContentAnalysisDelegate::Result& result) {
ASSERT_EQ(result.text_results.size(), 0u);
ASSERT_EQ(result.paths_results.size(), 2u);
ASSERT_TRUE(result.paths_results[0]);
ASSERT_TRUE(result.paths_results[1]);
called = true;
if (quit_closure.has_value())
quit_closure.value().Run();
}),
safe_browsing::DeepScanAccessPoint::UPLOAD);
run_loop.Run();
EXPECT_TRUE(called);
// No requests should be made since the DM token is unauthorized.
ASSERT_EQ(FakeBinaryUploadServiceStorage()->requests_count(), 0);
}
} // namespace enterprise_connectors } // namespace enterprise_connectors
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