Remove observer when removing delegate, in Instant.

Remove observer when removing delegate, in Instant.

I believe the crash (see bug link) is caused because we remove the delegate
connection, but we never stop observing the WebContents. I haven't reproduced
the crash, but I think this should fix the root cause. I've put in a bandaid as
well, which might be redundant, but good for safety. Will remove it once I can
confirm the fix works.

BUG=141875
R=sky@chromium.org
TEST=This type of crash (see bug) should go away.

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

[sreeram, 2012-08-10 19:32:48]

Please review.

[sreeram, 2012-08-10 22:03:10]

@sky seems to be away.

David/Shishir, could you review this quickly? I'd like to get this top crasher
fixed today.

[dominich, 2012-08-10 22:10:24]

LGTM

I think you could maybe wrap up the SetDelegate(x); x->Observe(); pairs of calls
in a utility but if you want to get this in to stop the crashing, it's fine as
is.

[Shishir, 2012-08-10 22:11:15]

lgtm

http://codereview.chromium.org/10829284/diff/2002/chrome/browser/instant/inst...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/10829284/diff/2002/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:232: if (!loader_->preview_contents()
||
How will you know what is fixing the issue: this if or the observer change?

[sreeram, 2012-08-10 22:13:54]

http://codereview.chromium.org/10829284/diff/2002/chrome/browser/instant/inst...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/10829284/diff/2002/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:232: if (!loader_->preview_contents()
||
On 2012/08/10 22:11:15, Shishir wrote:
> How will you know what is fixing the issue: this if or the observer change?

I'll know when debug builds don't crash (the DCHECK should never fire). Of
course, nobody runs debug builds, but perhaps on a try bot...

[sreeram, 2012-08-10 22:25:06]

PTAL. I followed @sky's excellent advice to just delete the delegate, instead of
futzing with Observe() calls.

[dominich, 2012-08-10 22:28:17]

LGTM

much better - did this show up on debug trybots before?

[sreeram, 2012-08-10 22:30:46]

On Fri, Aug 10, 2012 at 3:28 PM,  <dominich@chromium.org> wrote:
> much better - did this show up on debug trybots before?

Nope.

[dhollowa, 2012-08-10 22:32:21]

LG

[sky, 2012-08-10 22:35:39]

LGTM

http://codereview.chromium.org/10829284/diff/5002/chrome/browser/instant/inst...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/10829284/diff/5002/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:253: // If the page doesn't support the
Instant API, InstantController schedules
Can we move this above 250? Seem dangerous to do things after calling out.

[sreeram, 2012-08-10 22:38:48]

http://codereview.chromium.org/10829284/diff/5002/chrome/browser/instant/inst...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/10829284/diff/5002/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:253: // If the page doesn't support the
Instant API, InstantController schedules
On 2012/08/10 22:35:39, sky wrote:
> Can we move this above 250? Seem dangerous to do things after calling out.

Done.