Fix assert on Mandoline shutdown.

Fix assert on Mandoline shutdown.

Committed: https://crrev.com/f917affc6efda9d5368506cd43c063338d8544d5
Cr-Commit-Position: refs/heads/master@{#332881}

[jam, 2015-06-04 03:04:25]

jam@chromium.org changed reviewers:
+ sky@chromium.org

[jam, 2015-06-04 03:04:25]

Not really familiar with this code, so this is more of an attempt.

[Fady Samuel, 2015-06-04 03:21:45]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-06-04 03:21:45]

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc
File mandoline/tab/frame.cc (right):

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc#newc...
mandoline/tab/frame.cc:91: if (tree_->root() == this) {
Alternatively, FrameTree could hold a weakptr to the Frame? Thoughts?

[sky, 2015-06-04 03:38:00]

Can the browser destroy the frametree before we get here?

[jam, 2015-06-04 03:51:31]

On 2015/06/04 03:38:00, sky wrote:
> Can the browser destroy the frametree before we get here?

Frame::OnViewDestroying gets called before Browser's dtor or
Browser::OnViewManagerDestroyed. Where would be a place to call this?

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc
File mandoline/tab/frame.cc (right):

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc#newc...
mandoline/tab/frame.cc:91: if (tree_->root() == this) {
On 2015/06/04 03:21:45, Fady Samuel wrote:
> Alternatively, FrameTree could hold a weakptr to the Frame? Thoughts?

(I'll defer to Scott)

[sky, 2015-06-04 04:22:58]

What is the flow of shutdown now? Is it keyed off the ViewManager going away?

How about I trade you this bug for the android bug? :)

  -Scott

On Wed, Jun 3, 2015 at 8:51 PM,  <jam@chromium.org> wrote:
> On 2015/06/04 03:38:00, sky wrote:
>>
>> Can the browser destroy the frametree before we get here?
>
>
> Frame::OnViewDestroying gets called before Browser's dtor or
> Browser::OnViewManagerDestroyed. Where would be a place to call this?
>
>
> https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc
> File mandoline/tab/frame.cc (right):
>
>
https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc#newc...
> mandoline/tab/frame.cc:91: if (tree_->root() == this) {
> On 2015/06/04 03:21:45, Fady Samuel wrote:
>>
>> Alternatively, FrameTree could hold a weakptr to the Frame? Thoughts?
>
>
> (I'll defer to Scott)
>
> https://codereview.chromium.org/1153393007/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-06-04 19:17:50]

LGTM

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc
File mandoline/tab/frame.cc (right):

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc#newc...
mandoline/tab/frame.cc:92: view_ = nullptr;
view_->RemoveObserver(this) before the return

[jam, 2015-06-04 19:20:49]

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc
File mandoline/tab/frame.cc (right):

https://codereview.chromium.org/1153393007/diff/1/mandoline/tab/frame.cc#newc...
mandoline/tab/frame.cc:92: view_ = nullptr;
On 2015/06/04 19:17:50, sky wrote:
> view_->RemoveObserver(this) before the return

Done.

[jam, 2015-06-04 19:20:57]

The CQ bit was checked by jam@chromium.org

[jam, 2015-06-04 19:20:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1153393007/#ps20001
(title: "comment")

[commit-bot: I haz the power, 2015-06-04 19:22:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153393007/20001

[commit-bot: I haz the power, 2015-06-04 19:37:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-04 19:38:33]

Patchset 2 (id:??) landed as
https://crrev.com/f917affc6efda9d5368506cd43c063338d8544d5
Cr-Commit-Position: refs/heads/master@{#332881}