Fix crash bug http://crbug.com/725594

Fix crash bug http://crbug.com/725594

The order of destruction of frame_tree_ and rvh_delegate_view_ is relevant. This
change ensure frame_tree_ is destructed before rvh_delegate_view_.

BUG=chromium:725594, chromium:725402
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2903593003
Cr-Commit-Position: refs/heads/master@{#474201}
Committed: https://chromium.googlesource.com/chromium/src/+/8da026c9e77facffc5ed46cfb75c2932f09fcc12

[Hzj_jie, 2017-05-23 18:51:39]

Description was changed from

==========
Fix crash bug http://crbug.com/725594

The order of destruction of frame_tree_ and rvh_delegate_view_ is relevant. This
change ensure frame_tree_ is destructed before rvh_delegate_view_.

BUG=chromium:725594, chromium:725402
==========

to

==========
Fix crash bug http://crbug.com/725594

The order of destruction of frame_tree_ and rvh_delegate_view_ is relevant. This
change ensure frame_tree_ is destructed before rvh_delegate_view_.

BUG=chromium:725594, chromium:725402
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Hzj_jie, 2017-05-23 18:51:43]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2017-05-23 18:52:16]

Description was changed from

==========
Fix crash bug http://crbug.com/725594

The order of destruction of frame_tree_ and rvh_delegate_view_ is relevant. This
change ensure frame_tree_ is destructed before rvh_delegate_view_.

BUG=chromium:725594, chromium:725402
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix crash bug http://crbug.com/725594

The order of destruction of frame_tree_ and rvh_delegate_view_ is relevant. This
change ensure frame_tree_ is destructed before rvh_delegate_view_.

BUG=chromium:725594, chromium:725402
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Hzj_jie, 2017-05-23 18:52:16]

zijiehe@chromium.org changed reviewers:
+ avi@chromium.org

[commit-bot: I haz the power, 2017-05-23 18:52:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 20:00:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 20:00:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hzj_jie, 2017-05-23 20:03:19]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2017-05-23 20:03:23]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-23 20:03:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-05-23 21:16:32]

I wonder if this is indicative of a deeper ownership issue. I don't think I've
ever seen DeleteSoon used like this.

[Hzj_jie, 2017-05-23 21:24:36]

On 2017/05/23 21:16:32, Avi (ping after 24h) wrote:
> I wonder if this is indicative of a deeper ownership issue. I don't think I've
> ever seen DeleteSoon used like this.

I think the constructor of the frame_tree_ shows some potential risks: it sends
itself four times to the FrameTree, so the order of destruction of the
implementations is relevant.

But if you are talking about DeleteSoon(unique_ptr.release())? It looks like a
very common pattern to ensure an object owned by a unique_ptr can be deleted
with expected order.
https://cs.chromium.org/search/?q=deletesoon%5C(.*release%5C(%5C)%5C)+package...

[Avi (use Gerrit), 2017-05-23 21:26:14]

On 2017/05/23 21:24:36, Hzj_jie wrote:
> On 2017/05/23 21:16:32, Avi (ping after 24h) wrote:
> > I wonder if this is indicative of a deeper ownership issue. I don't think
I've
> > ever seen DeleteSoon used like this.
> 
> I think the constructor of the frame_tree_ shows some potential risks: it
sends
> itself four times to the FrameTree, so the order of destruction of the
> implementations is relevant.
> 
> But if you are talking about DeleteSoon(unique_ptr.release())? It looks like a
> very common pattern to ensure an object owned by a unique_ptr can be deleted
> with expected order.
>
https://cs.chromium.org/search/?q=deletesoon%5C(.*release%5C(%5C)%5C)+package...

So...

1. Can you file a bug against FrameTree then? That sounds like it is less robust
than it could be
2. I personally find that kinda terrifying. :/ LGTM then.

[commit-bot: I haz the power, 2017-05-23 21:35:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 21:36:00]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-05-23 21:38:55]

On 2017/05/23 21:26:14, Avi (ping after 24h) wrote:
> On 2017/05/23 21:24:36, Hzj_jie wrote:
> > On 2017/05/23 21:16:32, Avi (ping after 24h) wrote:
> > > I wonder if this is indicative of a deeper ownership issue. I don't think
> I've
> > > ever seen DeleteSoon used like this.
> > 
> > I think the constructor of the frame_tree_ shows some potential risks: it
> sends
> > itself four times to the FrameTree, so the order of destruction of the
> > implementations is relevant.
> > 
> > But if you are talking about DeleteSoon(unique_ptr.release())? It looks like
a
> > very common pattern to ensure an object owned by a unique_ptr can be deleted
> > with expected order.
> >
>
https://cs.chromium.org/search/?q=deletesoon%5C(.*release%5C(%5C)%5C)+package...
> 
> So...
> 
> 1. Can you file a bug against FrameTree then? That sounds like it is less
robust
> than it could be
> 2. I personally find that kinda terrifying. :/ LGTM then.

I tend to think this is an issue of InterstitialPageImpl class instead of
FrameTree. Since itself is the four delegates of the FrameTree, it looks quite
wrong to me to randomly destruct frame_tree_. Instead it should ensure
frame_tree_ is destructed before other delegates.
I will add more comments in the source code as well as the bug to explain the
root cause here.

[Hzj_jie, 2017-05-23 21:42:47]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2017-05-23 21:43:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-05-23 21:45:33]

The CQ bit was unchecked by zijiehe@chromium.org

[Hzj_jie, 2017-05-23 21:55:19]

On 2017/05/23 21:43:37, commit-bot: I haz the power wrote:
> CQ is trying da patch.
> 
> Follow status at:
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

I am working on a different solution by replacing "FrameTree frame_tree_" with
"std::unique_ptr<FrameTree> frame_tree_", and reset it in the destructor
manually.

[Hzj_jie, 2017-05-23 21:58:42]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 21:59:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Hzj_jie, 2017-05-23 22:41:54]

Avi, how about this solution?

[Avi (use Gerrit), 2017-05-23 22:44:15]

Nicer. LGTM with nit.

https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.cc:166: frame_tree_(new
FrameTree(new InterstitialPageNavigatorImpl(this,
base::MakeUnique<>

[Hzj_jie, 2017-05-23 22:57:21]

On 2017/05/23 22:44:15, Avi (ping after 24h) wrote:
> Nicer. LGTM with nit.
> 
>
https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/interstitial_page_impl.cc (right):
> 
>
https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
> content/browser/frame_host/interstitial_page_impl.cc:166: frame_tree_(new
> FrameTree(new InterstitialPageNavigatorImpl(this,
> base::MakeUnique<>

It seems like there is not benefit by using base::MakeUnique here. IMO,
MakeUnique is better for statements like,
std::unique_ptr<Type> p = base::MakeUnique<TypeImpl>(...);
or even,
auto p = base::MakeUnique<Type>(...);
or,
std::unique_ptr<Type> p;
p = base::MakeUnique<TypeImpl>(...);
because there is not std::unique_ptr::operator=(T*);

[Avi (use Gerrit), 2017-05-23 23:01:39]

On 2017/05/23 22:57:21, Hzj_jie wrote:
> On 2017/05/23 22:44:15, Avi (ping after 24h) wrote:
> > Nicer. LGTM with nit.
> > 
> >
>
https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
> > File content/browser/frame_host/interstitial_page_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
> > content/browser/frame_host/interstitial_page_impl.cc:166: frame_tree_(new
> > FrameTree(new InterstitialPageNavigatorImpl(this,
> > base::MakeUnique<>
> 
> It seems like there is not benefit by using base::MakeUnique here. IMO,
> MakeUnique is better for statements like,
> std::unique_ptr<Type> p = base::MakeUnique<TypeImpl>(...);
> or even,
> auto p = base::MakeUnique<Type>(...);
> or,
> std::unique_ptr<Type> p;
> p = base::MakeUnique<TypeImpl>(...);
> because there is not std::unique_ptr::operator=(T*);

We're trying to move away from raw new calls.
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

[Hzj_jie, 2017-05-23 23:20:14]

On 2017/05/23 23:01:39, Avi (ping after 24h) wrote:
> On 2017/05/23 22:57:21, Hzj_jie wrote:
> > On 2017/05/23 22:44:15, Avi (ping after 24h) wrote:
> > > Nicer. LGTM with nit.
> > > 
> > >
> >
>
https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
> > > File content/browser/frame_host/interstitial_page_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2903593003/diff/40001/content/browser/frame_h...
> > > content/browser/frame_host/interstitial_page_impl.cc:166: frame_tree_(new
> > > FrameTree(new InterstitialPageNavigatorImpl(this,
> > > base::MakeUnique<>
> > 
> > It seems like there is not benefit by using base::MakeUnique here. IMO,
> > MakeUnique is better for statements like,
> > std::unique_ptr<Type> p = base::MakeUnique<TypeImpl>(...);
> > or even,
> > auto p = base::MakeUnique<Type>(...);
> > or,
> > std::unique_ptr<Type> p;
> > p = base::MakeUnique<TypeImpl>(...);
> > because there is not std::unique_ptr::operator=(T*);
> 
> We're trying to move away from raw new calls.
>
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

Oh, got it. Thank you.

[Hzj_jie, 2017-05-23 23:23:17]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 23:23:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-05-23 23:32:15]

lgtm

Thank you.

[commit-bot: I haz the power, 2017-05-24 05:13:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 05:13:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Hzj_jie, 2017-05-24 06:29:55]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2017-05-24 06:30:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 07:49:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495607395481150,
"parent_rev": "b9b069b5a30a07c45fcfd0f1820427df40ebbb10", "commit_rev":
"8da026c9e77facffc5ed46cfb75c2932f09fcc12"}

[commit-bot: I haz the power, 2017-05-24 07:50:10]

Description was changed from

==========
Fix crash bug http://crbug.com/725594

The order of destruction of frame_tree_ and rvh_delegate_view_ is relevant. This
change ensure frame_tree_ is destructed before rvh_delegate_view_.

BUG=chromium:725594, chromium:725402
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix crash bug http://crbug.com/725594

The order of destruction of frame_tree_ and rvh_delegate_view_ is relevant. This
change ensure frame_tree_ is destructed before rvh_delegate_view_.

BUG=chromium:725594, chromium:725402
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2903593003
Cr-Commit-Position: refs/heads/master@{#474201}
Committed:
https://chromium.googlesource.com/chromium/src/+/8da026c9e77facffc5ed46cfb75c...
==========

[commit-bot: I haz the power, 2017-05-24 07:50:12]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8da026c9e77facffc5ed46cfb75c...