[DevTools] Migrate InputHandler from RenderWidgetHostImpl to RenderFrameHostImpl.

[DevTools] Migrate InputHandler from RenderWidgetHostImpl to RenderFrameHostImpl.

The lifetime of RenderWidgetHostImpl is not guaranteed and doesn't align
with our raw-pointer usage. Grabbing it through RenderFrameHostImpl works.

BUG=none

[dgozman, 2016-12-01 20:32:08]

dgozman@chromium.org changed reviewers:
+ pfeldman@chromium.org, samuong@chromium.org

[dgozman, 2016-12-01 20:32:08]

Could you please take a look?

[dgozman, 2016-12-01 20:33:12]

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

[commit-bot: I haz the power, 2016-12-01 20:33:46]

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

[samuong, 2016-12-01 20:56:26]

lgtm

https://codereview.chromium.org/2387353004/ needs to add and remove
InputEventObservers from the RWHI. If the RFHI can return different RWHIs each
time, would we run into a problem where we call AddInputEventObserver and
RemoveInputEventObserver on different RWHIs?

[commit-bot: I haz the power, 2016-12-01 21:34:12]

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

[commit-bot: I haz the power, 2016-12-01 21:34:13]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-12-01 21:59:02]

On 2016/12/01 20:56:26, samuong wrote:
> lgtm
> 
> https://codereview.chromium.org/2387353004/ needs to add and remove
> InputEventObservers from the RWHI. If the RFHI can return different RWHIs each
> time, would we run into a problem where we call AddInputEventObserver and
> RemoveInputEventObserver on different RWHIs?

Well, this approach works for us in page_handler and color_picker files. Could
you please apply this together with your patch and see whether it works?

[samuong, 2016-12-02 21:37:59]

Unfortunately this didn't work, so not lgtm for now:

http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

It's still crashing in the (now renamed) InputHandler::SetRenderFrameHost
function. From the stack trace, I notice this is getting called from the
~RenderFrameHostImpl destructor:

#0 0x0000031a9ee7 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7f55780aacb0 <unknown>
#2 0x000002508fee content::devtools::input::InputHandler::SetRenderFrameHost()
#3 0x00000252c590
content::RenderFrameDevToolsAgentHost::UpdateProtocolHandlers()
#4 0x00000252b528 content::RenderFrameDevToolsAgentHost::DiscardPending()
#5 0x00000252d9aa content::RenderFrameDevToolsAgentHost::DidFinishNavigation()
#6 0x0000028b90d7 content::WebContentsImpl::DidFinishNavigation()
#7 0x0000025ad85f content::NavigationHandleImpl::~NavigationHandleImpl()
#8 0x0000025ade99 content::NavigationHandleImpl::~NavigationHandleImpl()
#9 0x0000025b8490 content::RenderFrameHostImpl::~RenderFrameHostImpl()
#10 0x000002993ea0 content::TestRenderFrameHost::~TestRenderFrameHost()
...

Not sure if we're dereferencing a pointer to the same RFHI that's being
destructed, or a different one. Any ideas?

[samuong, 2016-12-02 23:06:03]

After running another try job with some extra debug statements, I've found that
we are dereferencing a pointer to the RFHI object that is being destructed.

Is this a test-only issue, or is this likely to come up in normal situations
too?

[samuong, 2016-12-06 21:44:01]

just wanted to follow up on this - can you let me know what you think here?

[dgozman, 2016-12-06 21:52:48]

dgozman@chromium.org changed reviewers:
+ clamy@chromium.org

[dgozman, 2016-12-06 21:52:49]

[+clamy]

Sorry, this one slipped through.

I think this is a real bug, which only happens with browser side navigation.
Surprisingly, we didn't hit it before, event in tests.

Camille, what do you think about this? We need to grab RFHI instance to clean up
at this point, but it's called from RFHI's destructor. Any ideas on the proper
way to clean up in this case? Is there any other notification called before RFHI
is destroyed which we can use instead? Or maybe that's just a test issue?

#0 0x0000031a9ee7 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7f55780aacb0 <unknown>
#2 0x000002508fee content::devtools::input::InputHandler::SetRenderFrameHost()
#3 0x00000252c590
content::RenderFrameDevToolsAgentHost::UpdateProtocolHandlers()
#4 0x00000252b528 content::RenderFrameDevToolsAgentHost::DiscardPending()
#5 0x00000252d9aa content::RenderFrameDevToolsAgentHost::DidFinishNavigation()
#6 0x0000028b90d7 content::WebContentsImpl::DidFinishNavigation()
#7 0x0000025ad85f content::NavigationHandleImpl::~NavigationHandleImpl()
#8 0x0000025ade99 content::NavigationHandleImpl::~NavigationHandleImpl()
#9 0x0000025b8490 content::RenderFrameHostImpl::~RenderFrameHostImpl()
#10 0x000002993ea0 content::TestRenderFrameHost::~TestRenderFrameHost()

[clamy, 2016-12-07 14:51:32]

Ok so as far as I understand, the stack trace you see corresponds to a very
precise case:
- you had a cross-site navigation for which you received a response but it has
not committed yet (this state is not supposed to last long in PlzNavigate but it
happens).
- it gets interrupted by the start of a new navigation -> this causes us to
delete the speculative RFH that was supposed to commit it and which was holding
the NavigationHandle. The deletion of the NavigationHandle at the end of the
RFHI dtor fires DidFinishNavigation.
- we then call DiscardPending (since the navigation in the speculative RFH did
not commit), which causes us to set the current RFH back in the protocol
handlers. If one of them tries to access the RFH that they had previously (ie
the speculative), then there would be a problem, since we're being called whild
it's half destroyed.

If that's the case, I would suggest checking in RenderFrameDeleted that the
frame deleted is pending RFH, and updating the protocol handlers at that time.
Normally this should happen before the end of RFHI dtor implementation.

[clamy, 2016-12-07 14:54:02]

On 2016/12/07 14:51:32, clamy wrote:
> Ok so as far as I understand, the stack trace you see corresponds to a very
> precise case:
> - you had a cross-site navigation for which you received a response but it has
> not committed yet (this state is not supposed to last long in PlzNavigate but
it
> happens).
> - it gets interrupted by the start of a new navigation -> this causes us to
> delete the speculative RFH that was supposed to commit it and which was
holding
> the NavigationHandle. The deletion of the NavigationHandle at the end of the
> RFHI dtor fires DidFinishNavigation.
> - we then call DiscardPending (since the navigation in the speculative RFH did
> not commit), which causes us to set the current RFH back in the protocol
> handlers. If one of them tries to access the RFH that they had previously (ie
> the speculative), then there would be a problem, since we're being called
whild
> it's half destroyed.
> 
> If that's the case, I would suggest checking in RenderFrameDeleted that the
> frame deleted is pending RFH, and updating the protocol handlers at that time.
> Normally this should happen before the end of RFHI dtor implementation.

Alternatively, we could also explicitly reset the NavigationHandle before
reaching the end of the dtor, which would allow DidFinishNavigation to be called
before the RFH has started to be destroyed. This might be cleaner actually,
since you may not be the only one having the issue.