Update FileSelectHelper to use RenderFrameHost notifications.

Update FileSelectHelper to use RenderFrameHost notifications.

The FileSelectHelper class is using RenderViewHost notifications to
ensure it cleans up itself. However, as I moved it to use
RenderFrameHost for file selection (but not directory enumeration yet),
it opened it up to use-after-free conditions as it was not monitoring
for those objects going away.

This CL fixes that and adds a test to ensure it gets properly cleaned up
when an iframe that requested the file chooser is removed.

BUG=621076
Committed: https://crrev.com/f6a80acd01b832cbaa4ec5d77d213dcb73ba70e1
Cr-Commit-Position: refs/heads/master@{#402683}

[nasko, 2016-06-27 23:24:46]

nasko@chromium.org changed reviewers:
+ thestig@chromium.org

[nasko, 2016-06-27 23:24:47]

Hey Lei,
Can you review this CL for me? It fixes an use-after-free bug I introduced when
I moved it from using RenderViewHost to using RenderFrameHost. I'm fixing it and
doing some renaming/cleanup.
Thanks in advance!
Nasko

[Lei Zhang, 2016-06-27 23:32:26]

lgtm

https://codereview.chromium.org/2102883002/diff/20001/chrome/browser/iframe_b...
File chrome/browser/iframe_browsertest.cc (right):

https://codereview.chromium.org/2102883002/diff/20001/chrome/browser/iframe_b...
chrome/browser/iframe_browsertest.cc:50: // Note: For the bug to occur, the
parent and child frame need to be in
Can we verify tab->GetMainFrame() and |frame| are in fact on the same site?

[nasko, 2016-06-27 23:49:00]

https://codereview.chromium.org/2102883002/diff/20001/chrome/browser/iframe_b...
File chrome/browser/iframe_browsertest.cc (right):

https://codereview.chromium.org/2102883002/diff/20001/chrome/browser/iframe_b...
chrome/browser/iframe_browsertest.cc:50: // Note: For the bug to occur, the
parent and child frame need to be in
On 2016/06/27 23:32:26, Lei Zhang wrote:
> Can we verify tab->GetMainFrame() and |frame| are in fact on the same site?

Done.

[nasko, 2016-06-27 23:49:19]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2016-06-27 23:50:22]

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, 2016-06-28 02:19:57]

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

[commit-bot: I haz the power, 2016-06-28 02:19:59]

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

[Lei Zhang, 2016-06-28 05:00:49]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-06-28 05:01:01]

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

[Lei Zhang, 2016-06-28 05:02:06]

On 2016/06/28 02:19:59, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Not sure why ASAN (LSAN) is giving you trouble. The code here isn't malloc'ing
anything directly. Push button, try again...

[commit-bot: I haz the power, 2016-06-28 06:34:52]

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

[commit-bot: I haz the power, 2016-06-28 06:34:53]

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

[nasko, 2016-06-28 21:38:57]

Hey Lei,
I've disabled the test until I can investigate why the memory leak is being
reported. I'm fairly confident in the fix, so I'd like to land this to prevent
crashes/corruption. What do you think?
Thanks in advance!
Nasko

[Lei Zhang, 2016-06-28 22:00:56]

That's unfortunate, but sure. Let's land it and keep an eye out for crash
reports.

https://codereview.chromium.org/2102883002/diff/60001/chrome/browser/iframe_b...
File chrome/browser/iframe_browsertest.cc (right):

https://codereview.chromium.org/2102883002/diff/60001/chrome/browser/iframe_b...
chrome/browser/iframe_browsertest.cc:44: IN_PROC_BROWSER_TEST_F(IFrameTest,
DISABLED_FileChooserInDestroyedSubframe) {
Please add a comment to explain why it's disabled.

[nasko, 2016-06-28 22:15:38]

Comment added.

[nasko, 2016-06-28 23:35:54]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-06-28 23:35:55]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2102883002/#ps80001
(title: "Comment on why test is disabled.")

[commit-bot: I haz the power, 2016-06-28 23:37:29]

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, 2016-06-29 02:37:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-29 02:40:34]

Description was changed from

==========
Update FileSelectHelper to use RenderFrameHost notifications.

The FileSelectHelper class is using RenderViewHost notifications to
ensure it cleans up itself. However, as I moved it to use
RenderFrameHost for file selection (but not directory enumeration yet),
it opened it up to use-after-free conditions as it was not monitoring
for those objects going away.

This CL fixes that and adds a test to ensure it gets properly cleaned up
when an iframe that requested the file chooser is removed.

BUG=621076
==========

to

==========
Update FileSelectHelper to use RenderFrameHost notifications.

The FileSelectHelper class is using RenderViewHost notifications to
ensure it cleans up itself. However, as I moved it to use
RenderFrameHost for file selection (but not directory enumeration yet),
it opened it up to use-after-free conditions as it was not monitoring
for those objects going away.

This CL fixes that and adds a test to ensure it gets properly cleaned up
when an iframe that requested the file chooser is removed.

BUG=621076

Committed: https://crrev.com/f6a80acd01b832cbaa4ec5d77d213dcb73ba70e1
Cr-Commit-Position: refs/heads/master@{#402683}
==========

[commit-bot: I haz the power, 2016-06-29 02:40:36]

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