0

Revert "[FIX] Remove memory leak in FileChooserChromeOsTest"

This reverts commit 029fa65abb.

Reason for revert: Enabled tests are still failing
FileChooserChromeOsTest.FileCancelationAllowed
FileChooserChromeOsTest.OnlyAllowSingleFileSelection
FileChooserChromeOsTest.SingleFileSelection

first failure:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/53270/overview


Original change's description:
> [FIX] Remove memory leak in FileChooserChromeOsTest
>
> Destroy file chooser dialog sooner, to prevent race condition on memory
> sanitized unittest.
>
> Bug: crbug.com/1453685
> Change-Id: I7577f979d1ad2e379f3325ea95119d6f1cec11f9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4774158
> Reviewed-by: Joe Downing <joedow@chromium.org>
> Commit-Queue: Ashutosh Singhal <macinashutosh@google.com>
> Cr-Commit-Position: refs/heads/main@{#1191348}

Bug: crbug.com/1453685
Change-Id: Id4a84632036e0cca7df8cb457b69fa5fd5c793bb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4835122
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Owen Min <zmin@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1191491}
This commit is contained in:
Owen Min
2023-09-01 18:35:10 +00:00
committed by Chromium LUCI CQ
parent bebb47b9d7
commit a96dde93d9
2 changed files with 24 additions and 14 deletions

@ -10,12 +10,16 @@
#include "base/files/file_path.h"
#include "base/location.h"
#include "base/memory/scoped_refptr.h"
#include "base/notreached.h"
#include "base/task/bind_post_task.h"
#include "base/task/sequenced_task_runner.h"
#include "base/threading/sequence_bound.h"
#include "remoting/base/string_resources.h"
#include "remoting/host/chromeos/ash_proxy.h"
#include "remoting/host/file_transfer/file_chooser.h"
#include "remoting/protocol/file_transfer_helpers.h"
#include "ui/aura/window.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/shell_dialogs/select_file_dialog.h"
#include "ui/shell_dialogs/select_file_policy.h"
#include "ui/shell_dialogs/selected_file_info.h"
@ -45,7 +49,7 @@ class FileChooserChromeOs::Core : public ui::SelectFileDialog::Listener {
private:
void RunCallback(const FileChooser::Result& result);
void CloseDialog();
void Cleanup();
scoped_refptr<ui::SelectFileDialog> select_file_dialog_;
const raw_ref<AshProxy, LeakedDanglingUntriaged | ExperimentalAsh> ash_;
@ -79,7 +83,7 @@ FileChooserChromeOs::Core::Core(ResultCallback callback)
callback_(std::move(callback)) {}
FileChooserChromeOs::Core::~Core() {
CloseDialog();
select_file_dialog_->ListenerDestroyed();
}
void FileChooserChromeOs::Core::FileSelected(const base::FilePath& path,
@ -101,17 +105,9 @@ void FileChooserChromeOs::Core::FileSelectionCanceled(void* params) {
}
void FileChooserChromeOs::Core::RunCallback(const FileChooser::Result& result) {
CloseDialog();
std::move(callback_).Run(result);
}
void FileChooserChromeOs::Core::CloseDialog() {
if (select_file_dialog_) {
select_file_dialog_->ListenerDestroyed();
select_file_dialog_.reset();
}
}
void FileChooserChromeOs::Core::Show() {
// Restrict file access to the native volumes on the device and USB drives.
// This will prevent the remote user from selecting files from mapped network

@ -4,10 +4,15 @@
#include "remoting/host/file_transfer/file_chooser_chromeos.h"
#include <algorithm>
#include <memory>
#include "base/check_deref.h"
#include "base/files/file_path.h"
#include "base/functional/bind.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/scoped_refptr.h"
#include "base/task/single_thread_task_runner.h"
#include "base/test/task_environment.h"
@ -18,7 +23,9 @@
#include "remoting/protocol/file_transfer_helpers.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/aura/env.h"
#include "ui/shell_dialogs/select_file_dialog.h"
#include "ui/shell_dialogs/select_file_dialog_factory.h"
using base::test::TestFuture;
@ -36,7 +43,7 @@ class FileChooserChromeOsTest : public testing::Test {
FileChooserChromeOsTest(const FileChooserChromeOsTest&) = delete;
FileChooserChromeOsTest& operator=(const FileChooserChromeOsTest&) = delete;
// `testing::Test` implementation:
// `testing::Test` implementation.
void TearDown() override { ui::SelectFileDialog::SetFactory(nullptr); }
void CreateAndShowFileChooser(FileChooser::ResultCallback callback) {
@ -71,7 +78,14 @@ class FileChooserChromeOsTest : public testing::Test {
base::test::SingleThreadTaskEnvironment task_environment_;
};
TEST_F(FileChooserChromeOsTest, SingleFileSelection) {
#if defined(LEAK_SANITIZER)
// TODO(https://crbug.com/1453685): Fix LeakSanitizer failure.
#define MAYBE_DISABLED(name) DISABLED_##name
#else
#define MAYBE_DISABLED(name) name
#endif
TEST_F(FileChooserChromeOsTest, MAYBE_DISABLED(SingleFileSelection)) {
base::test::TestFuture<FileChooser::Result> result_future;
SetResultFileSelectionFactory(kTestFilePath);
@ -81,7 +95,7 @@ TEST_F(FileChooserChromeOsTest, SingleFileSelection) {
EXPECT_EQ(result_future.Get().success(), kTestFilePath);
}
TEST_F(FileChooserChromeOsTest, FileCancelationAllowed) {
TEST_F(FileChooserChromeOsTest, MAYBE_DISABLED(FileCancelationAllowed)) {
base::test::TestFuture<FileChooser::Result> result_future;
SetCancelFileSelectionFactory();
@ -92,7 +106,7 @@ TEST_F(FileChooserChromeOsTest, FileCancelationAllowed) {
protocol::FileTransfer_Error_Type_CANCELED);
}
TEST_F(FileChooserChromeOsTest, OnlyAllowSingleFileSelection) {
TEST_F(FileChooserChromeOsTest, MAYBE_DISABLED(OnlyAllowSingleFileSelection)) {
base::test::TestFuture<FileChooser::Result> result_future;
SetResultFileSelectionFactory(kTestFilePath);