The Firefox3XImporter test was incorrectly deleting a ref counted object caus...

The Firefox3XImporter test was incorrectly deleting a ref counted object causing an ASSERT to fire
resulting in this test to randomly fail on the Vista dbg builder.

Fix is to get rid of the delete and wrap the allocated Firefox3Observer object in a scoped_refptr.

This fixes http://code.google.com/p/chromium/issues/detail?id=22884

Bug=22884
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27045

[Nicolas Sylvain, 2009-09-24 05:18:56]

LGTM

http://codereview.chromium.org/231012/diff/1/2
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/231012/diff/1/2#newcode839
Line 839: // This test is disabled, see bug 22884
please remove this comment

[kuchhal, 2009-09-24 16:15:01]

I am not sure this would work. This object will be owned by ImporterHost
(see scoped_refptr<ProfileWriter> in importer.h) if firefox profile data (in
internal repository) is checked out. I think this change would cause errors
on Google Chrome buildbots.
May be just wrapping observer in scoped_ptr in if block will do the trick.

On Wed, Sep 23, 2009 at 10:17 PM, <ananta@chromium.org> wrote:

> Reviewers: Nicolas Sylvain,
>
> Description:
> The Firefox3XImporter test was incorrectly deleting a ref counted object
> causing
> an ASSERT to fire
> resulting in this test to randomly fail on the Vista dbg builder.
>
> Fix is to get rid of the delete and wrap the allocated Firefox3Observer
> object
> in a scoped_refptr.
>
> This fixes http://code.google.com/p/chromium/issues/detail?id=22884
>
> Bug=22884
>
>
> Please review this at http://codereview.chromium.org/231012
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/importer/importer_unittest.cc
>
>
> Index: chrome/browser/importer/importer_unittest.cc
> ===================================================================
> --- chrome/browser/importer/importer_unittest.cc        (revision 27039)
> +++ chrome/browser/importer/importer_unittest.cc        (working copy)
> @@ -71,7 +71,6 @@
>       file_util::AppendToPath(&data_path, L"firefox3_searchplugins");
>       if (!file_util::PathExists(data_path)) {
>         // TODO(maruel):  Create search test data that we can open source!
> -        delete observer;
>         LOG(ERROR) << L"Missing internal test data";
>         return;
>       }
> @@ -838,14 +837,16 @@
>  };
>
>  // This test is disabled, see bug 22884
> -TEST_F(ImporterTest, DISABLED_Firefox30Importer) {
> -  Firefox3Observer* observer = new Firefox3Observer();
> -  Firefox3xImporterTest(L"firefox3_profile", observer, observer, true);
> +TEST_F(ImporterTest, Firefox30Importer) {
> +  scoped_refptr<Firefox3Observer> observer = new Firefox3Observer();
> +  Firefox3xImporterTest(L"firefox3_profile", observer.get(),
> observer.get(),
> +                        true);
>  }
>
>  TEST_F(ImporterTest, Firefox35Importer) {
>   bool import_search_engines = false;
> -  Firefox3Observer* observer = new
> Firefox3Observer(import_search_engines);
> -  Firefox3xImporterTest(L"firefox35_profile", observer, observer,
> +  scoped_refptr<Firefox3Observer> observer =
> +      new Firefox3Observer(import_search_engines);
> +  Firefox3xImporterTest(L"firefox35_profile", observer.get(),
> observer.get(),
>                         import_search_engines);
>  }
>
>
>

[iyengar, 2009-09-24 16:24:35]

The ImporterHost::StartImportSettings task should run before
the Firefox3xImporterTest function returns and it would graba reference on
the Firefox3Observer object?

-Ananta

On Thu, Sep 24, 2009 at 9:14 AM, Rahul Kuchhal <kuchhal@chromium.org> wrote:

> I am not sure this would work. This object will be owned by ImporterHost
> (see scoped_refptr<ProfileWriter> in importer.h) if firefox profile data (in
> internal repository) is checked out. I think this change would cause errors
> on Google Chrome buildbots.
> May be just wrapping observer in scoped_ptr in if block will do the trick.
>
>
> On Wed, Sep 23, 2009 at 10:17 PM, <ananta@chromium.org> wrote:
>
>> Reviewers: Nicolas Sylvain,
>>
>> Description:
>> The Firefox3XImporter test was incorrectly deleting a ref counted object
>> causing
>> an ASSERT to fire
>> resulting in this test to randomly fail on the Vista dbg builder.
>>
>> Fix is to get rid of the delete and wrap the allocated Firefox3Observer
>> object
>> in a scoped_refptr.
>>
>> This fixes http://code.google.com/p/chromium/issues/detail?id=22884
>>
>> Bug=22884
>>
>>
>> Please review this at http://codereview.chromium.org/231012
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>>  M     chrome/browser/importer/importer_unittest.cc
>>
>>
>> Index: chrome/browser/importer/importer_unittest.cc
>> ===================================================================
>> --- chrome/browser/importer/importer_unittest.cc        (revision 27039)
>> +++ chrome/browser/importer/importer_unittest.cc        (working copy)
>> @@ -71,7 +71,6 @@
>>       file_util::AppendToPath(&data_path, L"firefox3_searchplugins");
>>       if (!file_util::PathExists(data_path)) {
>>         // TODO(maruel):  Create search test data that we can open source!
>> -        delete observer;
>>         LOG(ERROR) << L"Missing internal test data";
>>         return;
>>       }
>> @@ -838,14 +837,16 @@
>>  };
>>
>>  // This test is disabled, see bug 22884
>> -TEST_F(ImporterTest, DISABLED_Firefox30Importer) {
>> -  Firefox3Observer* observer = new Firefox3Observer();
>> -  Firefox3xImporterTest(L"firefox3_profile", observer, observer, true);
>> +TEST_F(ImporterTest, Firefox30Importer) {
>> +  scoped_refptr<Firefox3Observer> observer = new Firefox3Observer();
>> +  Firefox3xImporterTest(L"firefox3_profile", observer.get(),
>> observer.get(),
>> +                        true);
>>  }
>>
>>  TEST_F(ImporterTest, Firefox35Importer) {
>>   bool import_search_engines = false;
>> -  Firefox3Observer* observer = new
>> Firefox3Observer(import_search_engines);
>> -  Firefox3xImporterTest(L"firefox35_profile", observer, observer,
>> +  scoped_refptr<Firefox3Observer> observer =
>> +      new Firefox3Observer(import_search_engines);
>> +  Firefox3xImporterTest(L"firefox35_profile", observer.get(),
>> observer.get(),
>>                         import_search_engines);
>>  }
>>
>>
>>
>

[kuchhal, 2009-09-24 16:48:22]

Correct, it should work. Sorry for the false alarm.

On Thu, Sep 24, 2009 at 9:24 AM, iyengar <iyengar@google.com> wrote:

> The ImporterHost::StartImportSettings task should run before
> the Firefox3xImporterTest function returns and it would grab a reference
> on the Firefox3Observer object?
>
> -Ananta
>
>
> On Thu, Sep 24, 2009 at 9:14 AM, Rahul Kuchhal <kuchhal@chromium.org>wrote:
>
>> I am not sure this would work. This object will be owned by ImporterHost
>> (see scoped_refptr<ProfileWriter> in importer.h) if firefox profile data (in
>> internal repository) is checked out. I think this change would cause errors
>> on Google Chrome buildbots.
>> May be just wrapping observer in scoped_ptr in if block will do the trick.
>>
>>
>> On Wed, Sep 23, 2009 at 10:17 PM, <ananta@chromium.org> wrote:
>>
>>> Reviewers: Nicolas Sylvain,
>>>
>>> Description:
>>> The Firefox3XImporter test was incorrectly deleting a ref counted object
>>> causing
>>> an ASSERT to fire
>>> resulting in this test to randomly fail on the Vista dbg builder.
>>>
>>> Fix is to get rid of the delete and wrap the allocated Firefox3Observer
>>> object
>>> in a scoped_refptr.
>>>
>>> This fixes http://code.google.com/p/chromium/issues/detail?id=22884
>>>
>>> Bug=22884
>>>
>>>
>>> Please review this at http://codereview.chromium.org/231012
>>>
>>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>
>>> Affected files:
>>>  M     chrome/browser/importer/importer_unittest.cc
>>>
>>>
>>> Index: chrome/browser/importer/importer_unittest.cc
>>> ===================================================================
>>> --- chrome/browser/importer/importer_unittest.cc        (revision 27039)
>>> +++ chrome/browser/importer/importer_unittest.cc        (working copy)
>>> @@ -71,7 +71,6 @@
>>>       file_util::AppendToPath(&data_path, L"firefox3_searchplugins");
>>>       if (!file_util::PathExists(data_path)) {
>>>         // TODO(maruel):  Create search test data that we can open
>>> source!
>>> -        delete observer;
>>>         LOG(ERROR) << L"Missing internal test data";
>>>         return;
>>>       }
>>> @@ -838,14 +837,16 @@
>>>  };
>>>
>>>  // This test is disabled, see bug 22884
>>> -TEST_F(ImporterTest, DISABLED_Firefox30Importer) {
>>> -  Firefox3Observer* observer = new Firefox3Observer();
>>> -  Firefox3xImporterTest(L"firefox3_profile", observer, observer, true);
>>> +TEST_F(ImporterTest, Firefox30Importer) {
>>> +  scoped_refptr<Firefox3Observer> observer = new Firefox3Observer();
>>> +  Firefox3xImporterTest(L"firefox3_profile", observer.get(),
>>> observer.get(),
>>> +                        true);
>>>  }
>>>
>>>  TEST_F(ImporterTest, Firefox35Importer) {
>>>   bool import_search_engines = false;
>>> -  Firefox3Observer* observer = new
>>> Firefox3Observer(import_search_engines);
>>> -  Firefox3xImporterTest(L"firefox35_profile", observer, observer,
>>> +  scoped_refptr<Firefox3Observer> observer =
>>> +      new Firefox3Observer(import_search_engines);
>>> +  Firefox3xImporterTest(L"firefox35_profile", observer.get(),
>>> observer.get(),
>>>                         import_search_engines);
>>>  }
>>>
>>>
>>>
>>
>