Skip to content

GitLab

T
tangled
Commit c9994e05 authored by ahendrickson@chromium.org's avatar ahendrickson@chromium.org
Browse files
Options

Fixed memory leaks in download manager unit tests. 

BUG=83638
TEST=Valgrind does not detect the leak.


Review URL: http://codereview.chromium.org/6990044

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@86484 0039d316-1c4b-4281-b951-d872f2087c98
parent 70725eb1
Hide whitespace changes
Inline Side-by-side
Showing with 33 additions and 89 deletions
chrome/browser/download/download_manager_unittest.cc
View file @ c9994e05
......@@ -302,34 +302,28 @@ TEST_F(DownloadManagerTest, StartDownload) {
kStartDownloadCases[i].prompt_for_download);
SelectFileObserver observer(download_manager_);
DownloadCreateInfo* info = new DownloadCreateInfo;
// Normally, the download system takes ownership of info, and is
// responsible for deleting it. In these unit tests, however, we
// don't call the function that deletes it, so we do so ourselves.
scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
info->download_id = static_cast<int>(i);
info->prompt_user_for_save_location = kStartDownloadCases[i].save_as;
info->url_chain.push_back(GURL(kStartDownloadCases[i].url));
info->mime_type = kStartDownloadCases[i].mime_type;
download_manager_->CreateDownloadItem(info);
download_manager_->CreateDownloadItem(info.get());
DownloadFile* download_file(new DownloadFile(info, download_manager_));
DownloadFile* download_file(
new DownloadFile(info.get(), download_manager_));
AddDownloadToFileManager(info->download_id, download_file);
download_file->Initialize(false);
download_manager_->StartDownload(info->download_id);
message_loop_.RunAllPending();
// NOTE: At this point, |ContinueDownloadWithPath| will have been run if
// we don't need to prompt the user, so |info| could have been destructed.
// This means that we can't check any of its values.
// However, SelectFileObserver will have recorded any attempt to open the
// SelectFileObserver will have recorded any attempt to open the
// select file dialog.
// Note that DownloadManager::FileSelectionCanceled() is never called.
EXPECT_EQ(kStartDownloadCases[i].expected_save_as,
observer.ShowedFileDialogForId(i));
// If the Save As dialog pops up, it never reached
// DownloadManager::ContinueDownloadWithPath(), and never deleted info or
// completed. This cleans up info.
// Note that DownloadManager::FileSelectionCanceled() is never called.
if (observer.ShowedFileDialogForId(i)) {
delete info;
}
}
}
......@@ -340,8 +334,10 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
using ::testing::Return;
for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDownloadRenameCases); ++i) {
// |info| will be destroyed in download_manager_.
DownloadCreateInfo* info(new DownloadCreateInfo);
// Normally, the download system takes ownership of info, and is
// responsible for deleting it. In these unit tests, however, we
// don't call the function that deletes it, so we do so ourselves.
scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
info->download_id = static_cast<int>(i);
info->prompt_user_for_save_location = false;
info->url_chain.push_back(GURL());
......@@ -350,7 +346,7 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
const FilePath new_path(kDownloadRenameCases[i].suggested_path);
MockDownloadFile* download_file(
new MockDownloadFile(info, download_manager_));
new MockDownloadFile(info.get(), download_manager_));
AddDownloadToFileManager(info->download_id, download_file);
// |download_file| is owned by DownloadFileManager.
......@@ -370,7 +366,7 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
download_file, &MockDownloadFile::TestMultipleRename,
2, new_path))));
}
download_manager_->CreateDownloadItem(info);
download_manager_->CreateDownloadItem(info.get());
int32* id_ptr = new int32;
*id_ptr = i; // Deleted in FileSelected().
......@@ -397,8 +393,10 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
using ::testing::Invoke;
using ::testing::Return;
// |info| will be destroyed in download_manager_.
DownloadCreateInfo* info(new DownloadCreateInfo);
// Normally, the download system takes ownership of info, and is
// responsible for deleting it. In these unit tests, however, we
// don't call the function that deletes it, so we do so ourselves.
scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
info->download_id = static_cast<int>(0);
info->prompt_user_for_save_location = false;
info->url_chain.push_back(GURL());
......@@ -408,7 +406,7 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
const FilePath cr_path(download_util::GetCrDownloadPath(new_path));
MockDownloadFile* download_file(
new MockDownloadFile(info, download_manager_));
new MockDownloadFile(info.get(), download_manager_));
AddDownloadToFileManager(info->download_id, download_file);
// |download_file| is owned by DownloadFileManager.
......@@ -417,7 +415,7 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
EXPECT_CALL(*download_file, Rename(cr_path)).WillOnce(Return(true));
download_manager_->CreateDownloadItem(info);
download_manager_->CreateDownloadItem(info.get());
DownloadItem* download = GetActiveDownloadItem(0);
ASSERT_TRUE(download != NULL);
......@@ -462,8 +460,10 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
using ::testing::Invoke;
using ::testing::Return;
// |info| will be destroyed in download_manager_.
DownloadCreateInfo* info(new DownloadCreateInfo);
// Normally, the download system takes ownership of info, and is
// responsible for deleting it. In these unit tests, however, we
// don't call the function that deletes it, so we do so ourselves.
scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
info->download_id = static_cast<int>(0);
info->prompt_user_for_save_location = false;
info->url_chain.push_back(GURL());
......@@ -473,7 +473,7 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
const FilePath cr_path(download_util::GetCrDownloadPath(new_path));
MockDownloadFile* download_file(
new MockDownloadFile(info, download_manager_));
new MockDownloadFile(info.get(), download_manager_));
AddDownloadToFileManager(info->download_id, download_file);
// |download_file| is owned by DownloadFileManager.
......@@ -482,7 +482,7 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
EXPECT_CALL(*download_file, Rename(cr_path)).WillOnce(Return(true));
download_manager_->CreateDownloadItem(info);
download_manager_->CreateDownloadItem(info.get());
DownloadItem* download = GetActiveDownloadItem(0);
ASSERT_TRUE(download != NULL);
......@@ -540,15 +540,17 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
EXPECT_NE(0, uniquifier);
download_util::AppendNumberToPath(&unique_new_path, uniquifier);
// |info| will be destroyed in download_manager_.
DownloadCreateInfo* info(new DownloadCreateInfo);
// Normally, the download system takes ownership of info, and is
// responsible for deleting it. In these unit tests, however, we
// don't call the function that deletes it, so we do so ourselves.
scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
info->download_id = static_cast<int>(0);
info->prompt_user_for_save_location = true;
info->url_chain.push_back(GURL());
info->is_dangerous_file = false;
info->is_dangerous_url = false;
download_manager_->CreateDownloadItem(info);
download_manager_->CreateDownloadItem(info.get());
DownloadItem* download = GetActiveDownloadItem(0);
ASSERT_TRUE(download != NULL);
......@@ -561,7 +563,7 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
// name has been chosen, so we need to initialize the download file
// properly.
DownloadFile* download_file(
new DownloadFile(info, download_manager_));
new DownloadFile(info.get(), download_manager_));
download_file->Rename(cr_path);
// This creates the .crdownload version of the file.
download_file->Initialize(false);
......
tools/heapcheck/suppressions.txt
View file @ c9994e05
......@@ -1610,52 +1610,3 @@
fun:net::ClientSocketPoolManager::InitSocketHandleForRawConnect
fun:notifier::ProxyResolvingClientSocket::ProcessProxyResolveDone
}
{
bug_83638a
Heapcheck:Leak
fun:DownloadManagerTest_*_Test::TestBody
fun:testing::internal::HandleSehExceptionsInMethodIfSupported
fun:testing::internal::HandleExceptionsInMethodIfSupported
fun:testing::Test::Run
}
{
bug_83638b
Heapcheck:Leak
fun:__gnu_cxx::new_allocator::allocate
fun:std::_Vector_base::_M_allocate
fun:std::vector::_M_insert_aux
fun:std::vector::push_back
fun:DownloadManagerTest_*_Test::TestBody
fun:testing::internal::HandleSehExceptionsInMethodIfSupported
fun:testing::internal::HandleExceptionsInMethodIfSupported
fun:testing::Test::Run
}
{
bug_83638c
Heapcheck:Leak
fun:__gnu_cxx::new_allocator::allocate
fun:std::string::_Rep::_S_create
fun:std::string::_Rep::_M_clone
fun:std::string::reserve
fun:bool ::InitCanonical
fun:GURL::GURL
fun:DownloadManagerTest_StartDownload_Test::TestBody
fun:testing::internal::HandleSehExceptionsInMethodIfSupported
fun:testing::internal::HandleExceptionsInMethodIfSupported
fun:testing::Test::Run
}
{
bug_83638d
Heapcheck:Leak
fun:__gnu_cxx::new_allocator::allocate
fun:std::string::_Rep::_S_create
fun:std::string::_M_mutate
fun:std::string::_M_replace_safe
fun:std::string::assign
fun:std::string::assign
fun:std::string::operator=
fun:DownloadManagerTest_StartDownload_Test::TestBody
fun:testing::internal::HandleSehExceptionsInMethodIfSupported
fun:testing::internal::HandleExceptionsInMethodIfSupported
fun:testing::Test::Run
}
tools/valgrind/memcheck/suppressions.txt
View file @ c9994e05
......@@ -4597,15 +4597,6 @@
fun:_ZN2v88internal20HGlobalValueNumberer41CollectSideEffectsOnPathsToDominatedBlockEPNS0_11HBasicBlockES3_
fun:_ZN2v88internal20HGlobalValueNumberer12AnalyzeBlockEPNS0_11HBasicBlockEPNS0_9HValueMapE
}
{
bug_83638
Memcheck:Leak
fun:_Znw*
fun:*DownloadManagerTest_*_Test8TestBodyEv
fun:_ZN7testing8internal38HandleSehExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS4_FS3_vEPKc
fun:_ZN7testing8internal35HandleExceptionsInMethodIfSupportedINS_4TestEvEET0_PT_MS4_FS3_vEPKc
}
#-----------------------------------------------------------------------
# These only occur on our Google workstations
{
......
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