[Offline pages] Stop observing web contents and reset to null after use. Only create a new web cont…

[Offline pages] Stop observing web contents and reset to null after use. Only create a new web contents when we need it.

Before this change, we will create a new webcontents in preparation for a request that might not come in for hours or days. During the waiting period, a number of things can happen which may result in things related to the webcontents to become stale and therefore crash when we actually do use it. One hypothesis is that a render process host might be created without any render processes attached to it, causing the host to be removed under certain memory pressures or when certain clean-up processes are triggered.

For better memory and freshness, we should probably only create a web contents when we actually need it anyway, and clean up everything after we use it.

BUG=714272, 709443
Review-Url: https://codereview.chromium.org/2866643005
Cr-Commit-Position: refs/heads/master@{#469906}
Committed: https://chromium.googlesource.com/chromium/src/+/e427c75396a3388f0b64915128a371bb1c5793f0

[chili, 2017-05-05 20:31:25]

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

[commit-bot: I haz the power, 2017-05-05 20:32:10]

Dry run: CQ is trying da patch.

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

[chili, 2017-05-05 20:37:14]

chili@chromium.org changed reviewers:
+ carlosk@chromium.org, dewittj@chromium.org, petewil@chromium.org

[chili, 2017-05-05 20:37:15]

Hopefully this will fix the crashing?

[Pete Williamson, 2017-05-05 21:00:03]

lgtm

[dewittj, 2017-05-05 21:26:16]

lgtm

Any way you can add a test?

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:289:
ResetState();
nit: ResetState will cause a double-free of the destroyed web contents if called
from here. Do you want to fix this in this CL or in another?

[chili, 2017-05-05 21:34:23]

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:289:
ResetState();
On 2017/05/05 21:26:16, dewittj wrote:
> nit: ResetState will cause a double-free of the destroyed web contents if
called
> from here. Do you want to fix this in this CL or in another?

I still can't think of a good way to fix this. With my latest change, we should
only end up here if something unexpected caused the web contents to be destroyed
from elsewhere.  If it's something unexpected causing the web contents to be
destroyed, then the webcontents we're holding on to (and also the
background_loader_contents) are going to be invalid too, which means we should
reset/clear them?

If we're calling reset on the background_loader_contents, that's going to init
destructor for webcontents again, but since we're holding on to a unique_ptr, we
can't avoid this.....

Alternatively, we can assume that if something unexpected happens, we're going
to be screwed anyway....?

[carlosk, 2017-05-05 21:49:48]

RS LGTM.

Can you also add more detailed explanation of the reasoning behind this change
either to this description or to one of the linked crbugs?

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:289:
ResetState();
On 2017/05/05 21:34:23, chili wrote:
> On 2017/05/05 21:26:16, dewittj wrote:
> > nit: ResetState will cause a double-free of the destroyed web contents if
> called
> > from here. Do you want to fix this in this CL or in another?
> 
> I still can't think of a good way to fix this. With my latest change, we
should
> only end up here if something unexpected caused the web contents to be
destroyed
> from elsewhere.  If it's something unexpected causing the web contents to be
> destroyed, then the webcontents we're holding on to (and also the
> background_loader_contents) are going to be invalid too, which means we should
> reset/clear them?
> 
> If we're calling reset on the background_loader_contents, that's going to init
> destructor for webcontents again, but since we're holding on to a unique_ptr,
we
> can't avoid this.....
> 
> Alternatively, we can assume that if something unexpected happens, we're going
> to be screwed anyway....?

Can you detect when that situation is happening -- maybe even adding some flags
specifically for that -- and then force the crash so that we better understand
what's going on?

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

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

[commit-bot: I haz the power, 2017-05-05 22:14:24]

Dry run: This issue passed the CQ dry run.

[chili, 2017-05-05 22:34:37]

Description was changed from

==========
[Offline pages] Stop observing web contents and reset to null after use. Only
create a new web contents when we need it.

BUG=714272,709443
==========

to

==========
[Offline pages] Stop observing web contents and reset to null after use. Only
create a new web contents when we need it.

Before this change, we will create a new webcontents in preparation for a
request that might not come in for hours or days. During the waiting period, a
number of things can happen which may result in things related to the
webcontents to become stale and therefore crash when we actually do use it. One
hypothesis is that a render process host might be created without any render
processes attached to it, causing the host to be removed under certain memory
pressures or when certain clean-up processes are triggered.

For better memory and freshness, we should probably only create a web contents
when we actually need it anyway, and clean up everything after we use it.

BUG=714272,709443
==========

[chili, 2017-05-06 00:21:17]

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2866643005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/background_loader_offliner.cc:289:
ResetState();
On 2017/05/05 21:49:48, carlosk wrote:
> On 2017/05/05 21:34:23, chili wrote:
> > On 2017/05/05 21:26:16, dewittj wrote:
> > > nit: ResetState will cause a double-free of the destroyed web contents if
> > called
> > > from here. Do you want to fix this in this CL or in another?
> > 
> > I still can't think of a good way to fix this. With my latest change, we
> should
> > only end up here if something unexpected caused the web contents to be
> destroyed
> > from elsewhere.  If it's something unexpected causing the web contents to be
> > destroyed, then the webcontents we're holding on to (and also the
> > background_loader_contents) are going to be invalid too, which means we
should
> > reset/clear them?
> > 
> > If we're calling reset on the background_loader_contents, that's going to
init
> > destructor for webcontents again, but since we're holding on to a
unique_ptr,
> we
> > can't avoid this.....
> > 
> > Alternatively, we can assume that if something unexpected happens, we're
going
> > to be screwed anyway....?
> 
> Can you detect when that situation is happening -- maybe even adding some
flags
> specifically for that -- and then force the crash so that we better understand
> what's going on?

We currently don't have a good way of figuring out *when* or *if* this happens. 
Presumably, a gpu or rendering error can cause the web contents to be destroyed
though I haven't been able to reproduce in a test.

As I understand it, we might not even need this method because something
somewhere would've been terribly wrong if the web contents was destroyed without
us (the offliner) knowing about it first...

[chili, 2017-05-08 03:30:25]

The CQ bit was checked by chili@chromium.org

[commit-bot: I haz the power, 2017-05-08 03:30:44]

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-08 04:11:52]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1494214225220360, "parent_rev":
"ca849f94de7ba7696ba0f6a06438e69c0ebdeba7", "commit_rev":
"e427c75396a3388f0b64915128a371bb1c5793f0"}

[commit-bot: I haz the power, 2017-05-08 04:12:19]

Description was changed from

==========
[Offline pages] Stop observing web contents and reset to null after use. Only
create a new web contents when we need it.

Before this change, we will create a new webcontents in preparation for a
request that might not come in for hours or days. During the waiting period, a
number of things can happen which may result in things related to the
webcontents to become stale and therefore crash when we actually do use it. One
hypothesis is that a render process host might be created without any render
processes attached to it, causing the host to be removed under certain memory
pressures or when certain clean-up processes are triggered.

For better memory and freshness, we should probably only create a web contents
when we actually need it anyway, and clean up everything after we use it.

BUG=714272,709443
==========

to

==========
[Offline pages] Stop observing web contents and reset to null after use. Only
create a new web contents when we need it.

Before this change, we will create a new webcontents in preparation for a
request that might not come in for hours or days. During the waiting period, a
number of things can happen which may result in things related to the
webcontents to become stale and therefore crash when we actually do use it. One
hypothesis is that a render process host might be created without any render
processes attached to it, causing the host to be removed under certain memory
pressures or when certain clean-up processes are triggered.

For better memory and freshness, we should probably only create a web contents
when we actually need it anyway, and clean up everything after we use it.

BUG=714272,709443

Review-Url: https://codereview.chromium.org/2866643005
Cr-Commit-Position: refs/heads/master@{#469906}
Committed:
https://chromium.googlesource.com/chromium/src/+/e427c75396a3388f0b64915128a3...
==========

[commit-bot: I haz the power, 2017-05-08 04:12:20]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/e427c75396a3388f0b64915128a3...