Eliminate dependence of GoogleURLTracker et al. on InfoBarService

Eliminate dependence of GoogleURLTracker et al. on InfoBarService

This CL changes the dependence of GoogleURLTracker(InfoBarDelegate, MapEntry)
on InfoBarService to instead be a dependence on infobars::InfoBarManager. The
only reason that InfoBarManager wasn't sufficient was that
GoogleURLTrackerInfoBarDelegate obtained the WebContents from the
InfoBarService. This flow is replaced by introducing an OpenURL() API on
GoogleURLTrackerNavigationHelper (GoogleURLTrackerNavigationHelperImpl
implements this API by calling through to its WebContents instance, which is
the same instance associated with the InfoBarService).

BUG=373231, 373233
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273821

[Peter Kasting, 2014-05-19 13:59:21]

https://codereview.chromium.org/293503003/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_infobar_delegate.cc (right):

https://codereview.chromium.org/293503003/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_infobar_delegate.cc:65:
navigation_helper->OpenURL(new_search_url, CURRENT_TAB, false);
I don't think this is safe.

RemoveSelf() above can cause GoogleURLTrackerMapEntry::Observe() to be
synchronously notified with NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED if the
infobar isn't currently visible (e.g. it's in a background tab).  That in turn
deletes the map entry, which owns the navigation helper you're dereferencing
here.

You probably didn't see this if you manually tested because if the infobar _is_
currently visible, the notification will be asynchronous rather than
synchronous, and everything will work.

Either you need to use refcounting, or you need to change how the navigation
helper is owned, or you need to figure out a different way to plumb this
OpenURL() call.

[blundell, 2014-05-19 14:06:54]

https://codereview.chromium.org/293503003/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_infobar_delegate.cc (right):

https://codereview.chromium.org/293503003/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_infobar_delegate.cc:65:
navigation_helper->OpenURL(new_search_url, CURRENT_TAB, false);
Oof, yes you're right. I'm inclined to go back to the approach of having the
NavigationHelper be a WebContentsUserData. Thoughts?

 On 2014/05/19 13:59:22, Peter Kasting wrote:
> I don't think this is safe.
> 
> RemoveSelf() above can cause GoogleURLTrackerMapEntry::Observe() to be
> synchronously notified with NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED if the
> infobar isn't currently visible (e.g. it's in a background tab).  That in turn
> deletes the map entry, which owns the navigation helper you're dereferencing
> here.
> 
> You probably didn't see this if you manually tested because if the infobar
_is_
> currently visible, the notification will be asynchronous rather than
> synchronous, and everything will work.
> 
> Either you need to use refcounting, or you need to change how the navigation
> helper is owned, or you need to figure out a different way to plumb this
> OpenURL() call.

[Peter Kasting, 2014-05-19 14:16:25]

https://codereview.chromium.org/293503003/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_infobar_delegate.cc (right):

https://codereview.chromium.org/293503003/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_infobar_delegate.cc:65:
navigation_helper->OpenURL(new_search_url, CURRENT_TAB, false);
On 2014/05/19 14:06:54, blundell wrote:
> Oof, yes you're right. I'm inclined to go back to the approach of having the
> NavigationHelper be a WebContentsUserData. Thoughts?

Off-the-cuff ideas, you should second-guess me as I don't have all the object
lifetimes paged in to my brain:

Seems like another way would be, instead of being given the navigation helper in
the constructor, instead get a pointer to the map entry; then in this function,
call a map entry function that returns a
scoped_ptr<GoogleURLTrackerNavigationHelper> -- in other words, take the nav
helper away from the map entry, and hold it in a local variable in this
function.  That will keep it alive long enough to call OpenURL() on.

Yet another way might be for the map entry to pass over ownership of the
navigation helper at the time this infobar delegate is created -- from then on,
anyone who wants the nav helper gets it from the infobar.  (Maybe the map entry
has a helper function that does this access if necessary to shield the details
from callers?)  I haven't thought much about who needs to get at the nav helper
when to determine what the simplest model is.

You could, of course, go all the way back to WebContentsUserData, but I hate
stuffing things there -- it feels like a dumping ground, and it sucks having
this long-lived class existing on every web contents just on the off chance you
need a copy here.

[blundell, 2014-05-26 15:59:20]

Hi Peter,

This PS has the MapEntry pass the NavigationHelper to the InfoBarDelegate when
the InfoBarDelegate is set on the MapEntry. As GoogleURLTracker still needs to
get the navigation helper from the MapEntry, the MapEntry then calls through to
the InfoBarDelegate to supply the navigation helper in its accessor. The only
nasty thing is that in the corner case where we have to take ownership of the
NavHelper away from the InfoBarDelegate to allow it to outlive the delegate, the
GoogleURLTracker still needs to get the NavHelper from the MapEntry in the
teardown sequence. To facilitate this, the InfoBarDelegate stores the NavHelper
as a weak pointer in addition to the strong pointer. Let me know if you see
anything better.

I did testing with GoogleURLTrackerNavigationHelperImpl setting its
web_contents_ ivar to NULL in its destructor and verified that the problematic
case (accepting a switch of google TLD and thus kicking off a redo of all
searches) crashed in the prior version of this CL and works fine in the new PS.

[Peter Kasting, 2014-05-28 02:17:40]

I will try to review this tomorrow.

[Peter Kasting, 2014-05-28 22:19:04]

LGTM

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_infobar_delegate.h (right):

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_infobar_delegate.h:71: // In certain
corner cases, this object gives up ownership of
Nit: "In certain corner cases" -> "During Close()"?

(It just seems like not-a-corner-case)

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_infobar_delegate.h:73: // other classes
still require that |navigation_helper()| return a pointer to
Nit: No || around function name

Maybe "Sometimes after this point, other classes still attempt to call
navigation_helper() to access the (still-valid) instance."

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_map_entry.cc (right):

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_map_entry.cc:47: // certain cases.
Nit: Perhaps say what those cases are

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_map_entry.h (right):

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_map_entry.h:36: // delegate in
|SetInfoBarDelegate()|.
Nit: No || around function name

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_map_entry.h:39: return
navigation_helper_.get();
Nit: Shorter:

    return has_infobar_delegate() ?
        infobar_delegate_->navigation_helper() : navigation_helper_.get();

[blundell, 2014-05-30 09:23:52]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-30 09:24:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/293503003/100001

[blundell, 2014-05-30 09:26:18]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2014-05-30 09:39:17]

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_infobar_delegate.h (right):

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_infobar_delegate.h:71: // In certain
corner cases, this object gives up ownership of
On 2014/05/28 22:19:04, Peter Kasting wrote:
> Nit: "In certain corner cases" -> "During Close()"?
> 
> (It just seems like not-a-corner-case)

Done.

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_infobar_delegate.h:73: // other classes
still require that |navigation_helper()| return a pointer to
On 2014/05/28 22:19:04, Peter Kasting wrote:
> Nit: No || around function name
> 
> Maybe "Sometimes after this point, other classes still attempt to call
> navigation_helper() to access the (still-valid) instance."

Done.

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_map_entry.cc (right):

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_map_entry.cc:47: // certain cases.
On 2014/05/28 22:19:04, Peter Kasting wrote:
> Nit: Perhaps say what those cases are

Done.

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_map_entry.h (right):

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_map_entry.h:36: // delegate in
|SetInfoBarDelegate()|.
On 2014/05/28 22:19:04, Peter Kasting wrote:
> Nit: No || around function name

Done.

https://codereview.chromium.org/293503003/diff/80001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_map_entry.h:39: return
navigation_helper_.get();
On 2014/05/28 22:19:04, Peter Kasting wrote:
> Nit: Shorter:
> 
>     return has_infobar_delegate() ?
>         infobar_delegate_->navigation_helper() : navigation_helper_.get();

Done.

[blundell, 2014-05-30 09:39:20]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-30 09:41:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/293503003/120001

[commit-bot: I haz the power, 2014-05-30 13:25:10]

Change committed as 273821