Don’t call functions on a possibly-deleted object

Don’t call functions on a possibly-deleted object

JavaScriptDialogClosed walks up a list of frames. It shouldn’t be deleting frames and then calling functions on them.

BUG=717410
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2858743003
Cr-Commit-Position: refs/heads/master@{#469181}
Committed: https://chromium.googlesource.com/chromium/src/+/44acfaa84e1b59c9cda18fc9c9b8a2cd5dc35564

[Avi (use Gerrit), 2017-05-03 02:26:13]

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

[Avi (use Gerrit), 2017-05-03 02:26:21]

Description was changed from

==========
Don’t call functions on a possibly-deleted object

JavaScriptDialogClosed walks up a list of frames. It shouldn’t be deleting
frames and then calling functions on them.

BUG=717410
==========

to

==========
Don’t call functions on a possibly-deleted object

JavaScriptDialogClosed walks up a list of frames. It shouldn’t be deleting
frames and then calling functions on them.

BUG=717410
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-03 02:26:35]

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-03 02:47:53]

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

[Avi (use Gerrit), 2017-05-03 02:47:53]

I have a feeling you're gonna want to talk to me about this, especially since
you were the reviewer for the original bit of this code
(https://codereview.chromium.org/3621009)

[commit-bot: I haz the power, 2017-05-03 03:55:31]

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

[commit-bot: I haz the power, 2017-05-03 03:55:32]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-05-03 15:27:10]

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

[commit-bot: I haz the power, 2017-05-03 15:27:31]

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-03 16:32:49]

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

[commit-bot: I haz the power, 2017-05-03 16:32:50]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-05-03 21:33:47]

On 2017/05/03 02:47:53, Avi (ping after 24h) wrote:
> I have a feeling you're gonna want to talk to me about this, especially since
> you were the reviewer for the original bit of this code
> (https://codereview.chromium.org/3621009)

Thanks-- we had a good chat offline.

This LGTM.  If you are able to figure out repro steps without too much effort,
it would be great to include a test.  I'm not having any luck, though.  (I tried
--force-fieldtrials=AutoDismissingDialogs/Disabled --site-per-process, visiting
http://csreis.github.io/tests/cross-site-iframe.html, and clicking "Go
cross-site (simple page)."  I can't seem to trigger the crash by giving the
OOPIF a beforeunload and suppressing dialogs, though, if that was the theory.)

I'm ok with proceeding if we can't figure it out, with the caveat that the lack
of test is probably why https://crbug.com/37324 regressed.  :)

https://codereview.chromium.org/2858743003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2858743003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4368: //
http://crbug.com/288961 ). The only safe thing to do here is return.
Good find that this used to happen in
RenderFrameHostImpl::JavaScriptDialogClosed (due to https://crrev.com/61829). 
And yes, it doesn't happen anymore because dialogs aren't allowed during
beforeunload.

[Avi (use Gerrit), 2017-05-03 22:48:40]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2017-05-03 22:49:31]

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-03 22:55:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493851720341570,
"parent_rev": "2f60799d8cb1d8a99ba1b3a04156dc30de276272", "commit_rev":
"44acfaa84e1b59c9cda18fc9c9b8a2cd5dc35564"}

[commit-bot: I haz the power, 2017-05-03 22:55:46]

Description was changed from

==========
Don’t call functions on a possibly-deleted object

JavaScriptDialogClosed walks up a list of frames. It shouldn’t be deleting
frames and then calling functions on them.

BUG=717410
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Don’t call functions on a possibly-deleted object

JavaScriptDialogClosed walks up a list of frames. It shouldn’t be deleting
frames and then calling functions on them.

BUG=717410
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2858743003
Cr-Commit-Position: refs/heads/master@{#469181}
Committed:
https://chromium.googlesource.com/chromium/src/+/44acfaa84e1b59c9cda18fc9c9b8...
==========

[commit-bot: I haz the power, 2017-05-03 22:55:47]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/44acfaa84e1b59c9cda18fc9c9b8...

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

https://codereview.chromium.org/2858743003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/2858743003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4368: //
http://crbug.com/288961 ). The only safe thing to do here is return.
On 2017/05/03 21:33:46, Charlie Reis wrote:
> Good find that this used to happen in
> RenderFrameHostImpl::JavaScriptDialogClosed (due to https://crrev.com/61829). 
> And yes, it doesn't happen anymore because dialogs aren't allowed during
> beforeunload.

2017 me is super grateful to 2013 me for the hint.