Skip to content

GitLab

T
tangled
Commit 2ff1a6df authored by Xida Chen's avatar Xida Chen Committed by Chromium LUCI CQ
Browse files
Options

Revert "Handle sync paste deep scanning crash" 

This reverts commit 85f5cc53.

Reason for revert:
Test fails here:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tester%20(Ozone%20Wayland)/4513/overview

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}

TBR=anthonyvd@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,domfc@chromium.org

Change-Id: Ie52bf5a7b9aeaf3871795e0c480cf86d6be8fc0c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1166233
Bug: 1028133
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637406Reviewed-by: default avatarXida Chen <xidachen@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844853}
parent e146ff46
Hide whitespace changes
Inline Side-by-side
Showing with 3 additions and 168 deletions
chrome/browser/enterprise/connectors/content_analysis_delegate.cc
View file @ 2ff1a6df
......@@ -479,10 +479,6 @@ bool ContentAnalysisDelegate::UploadData() {
for (const base::FilePath& path : data_.paths)
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();
}
......@@ -588,9 +584,8 @@ void ContentAnalysisDelegate::MaybeCompleteScanRequest() {
if (final_result_ != FinalResult::WARNING)
RunCallback();
if (!UpdateDialog() && data_uploaded_) {
// No UI was shown. Delete |this| to cleanup, unless UploadData isn't done
// yet.
if (!UpdateDialog()) {
// No UI was shown. Delete |this| to cleanup.
delete this;
}
}
......
chrome/browser/enterprise/connectors/content_analysis_delegate.h
View file @ 2ff1a6df
......@@ -244,8 +244,7 @@ class ContentAnalysisDelegate {
private:
// Uploads data for deep scanning. Returns true if uploading is occurring in
// the background and false if there is nothing to do. Sets |data_uploaded_|
// to true right before returning.
// the background and false if there is nothing to do.
bool UploadData();
// Prepares an upload request for the text in |data_|. If |data_.text| is
......@@ -354,10 +353,6 @@ class ContentAnalysisDelegate {
// Scanning result to be shown to the user once every request is done.
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::WeakPtrFactory<ContentAnalysisDelegate> weak_ptr_factory_{this};
......
chrome/browser/enterprise/connectors/content_analysis_delegate_browsertest.cc
View file @ 2ff1a6df
......@@ -833,159 +833,4 @@ IN_PROC_BROWSER_TEST_P(ContentAnalysisDelegateBlockingSettingBrowserTest,
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
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