Fix memory leak in AutocompleteProviderTest::GetDestinationURL unit test.

Fix memory leak in AutocompleteProviderTest::GetDestinationURL unit test.
Handle if search_provider == NULL when constructing aqs CGI parameter.

BUG=175428
TEST=AutocompleteProviderTest::GetDestinationURL

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181973

[H Fung, 2013-02-12 00:26:14]

Sorry, I realized that I didn't handle search_provider_ being NULL, and that I
could have used a better initialization function for the test after I got the
bug about the memory leak in the test.

Since I'm not sure if search_provider_ == NULL will happen and cause crashes, I
might need to also request a merge?

[H Fung, 2013-02-12 00:27:36]

Regarding the memory leak, I did not figure out how to get heapcheck to fail,
but I tested by using valgrind with this command:

GYP_DEFINES="linux_use_tcmalloc=1 linux_use_heapchecker=1
linux_keep_shadow_stacks=1 werror= component=static_library"
tools/valgrind/chrome_tests.sh -t unit --gtest_filter=AutocompleteProviderTest.*

[Bart N., 2013-02-12 00:30:34]

https://codereview.chromium.org/12218104/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/12218104/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:260:
controller_->providers_.swap(providers);
I know it's not your fault, but the existing code makes assumption that
providers are NULL.

Can we fix it by either deleting pointers in controller_->providers_ or by
simply adding a DCHECK(controller_->providers_.empty())?

[H Fung, 2013-02-12 01:01:06]

https://codereview.chromium.org/12218104/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/12218104/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:260:
controller_->providers_.swap(providers);
On 2013/02/12 00:30:34, Bart N. wrote:
> I know it's not your fault, but the existing code makes assumption that
> providers are NULL.
> 
> Can we fix it by either deleting pointers in controller_->providers_ or by
> simply adding a DCHECK(controller_->providers_.empty())?

Done.

[Peter Kasting, 2013-02-12 01:34:49]

LGTM

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.cc:468: search_provider_ ?
Simpler:

  search_provider_ && search_provider_->field_trial_triggered_in_session()

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:260:
EXPECT_TRUE(controller_->providers_.empty());
Why don't we just use one of the STL util functions to free the elements anyway?
 Then there's no need for any explanation and the code is more future-proof.

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:601:
EXPECT_TRUE(controller_->search_provider_->
Nit: I'd break after ( instead of ->

[H Fung, 2013-02-12 02:19:44]

Thanks again Peter for the quick review!

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.cc:468: search_provider_ ?
On 2013/02/12 01:34:49, Peter Kasting wrote:
> Simpler:
> 
>   search_provider_ && search_provider_->field_trial_triggered_in_session()

Done.

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:260:
EXPECT_TRUE(controller_->providers_.empty());
On 2013/02/12 01:34:49, Peter Kasting wrote:
> Why don't we just use one of the STL util functions to free the elements
anyway?
>  Then there's no need for any explanation and the code is more future-proof.

I tried to use STLDeleteElements(&controller_->providers_);
but unfortunately, ~AutocompleteProvider() is protected.  I could copy-paste
that code to do nothing, but I didn't think it was worth it.

https://codereview.chromium.org/12218104/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:601:
EXPECT_TRUE(controller_->search_provider_->
On 2013/02/12 01:34:49, Peter Kasting wrote:
> Nit: I'd break after ( instead of ->

Done.

[commit-bot: I haz the power, 2013-02-12 02:23:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hfung@chromium.org/12218104/2003

[commit-bot: I haz the power, 2013-02-12 03:31:52]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 05:27:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hfung@chromium.org/12218104/2003

[commit-bot: I haz the power, 2013-02-12 06:30:18]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 17:34:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hfung@chromium.org/12218104/2003

[commit-bot: I haz the power, 2013-02-12 19:28:37]

Change committed as 181973