[DevTools] Rework RFDTAH navigation tracking with browser side navigation

[DevTools] Rework RFDTAH navigation tracking with browser side navigation

This patch introduces a separate code path for browser side navigation. Instead of
using pending and current, we keep track of a single RenderFrameHostImpl instance via
standard WebContentsObserver notifications.

While any navigations are active (determined by the navigation_handles_ set), we put messages
to suspended_messages_ instead of sending them to the renderer, resuming once navigations
commit to a specific RenderFrameHostImpl instance.

While navigation is not active, we send messages to renderer and put them to
waiting_for_response_messages_, which are exactly the messages received after last
state cookie update (see DevToolsMessageChunkProcessor).

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

Review-Url: https://codereview.chromium.org/2927983004
Cr-Commit-Position: refs/heads/master@{#478774}
Committed: https://chromium.googlesource.com/chromium/src/+/7e41e3eccc474549778339d62ff5d55483894f68

[dgozman, 2017-06-09 22:25:30]

Description was changed from

==========
[DevTools] Rework RFDTAH navigation tracking with browser side navigation

BUG=727517
==========

to

==========
[DevTools] Rework RFDTAH navigation tracking with browser side navigation

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

[dgozman, 2017-06-09 22:30:58]

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

[commit-bot: I haz the power, 2017-06-09 22:31:19]

Dry run: CQ is trying da patch.

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

[dgozman, 2017-06-09 22:37:06]

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:189:
ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadRawCookies(
This should be removed.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:195: bool
process_has_agents = false;
And this (it got moved to RevokePolicy(host)).

[dgozman, 2017-06-09 22:41:49]

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

[dgozman, 2017-06-09 22:41:50]

Could you please take a look? This is full of fprintfs and wrong formatting, but
good enough for logic review.

[commit-bot: I haz the power, 2017-06-09 22:42:03]

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

[commit-bot: I haz the power, 2017-06-09 22:42:04]

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

[caseq, 2017-06-10 00:02:46]

Mostly looks good, but I'm concerned of properly managing of frame_host_.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:224:
sent_messages_[call_id] = { session_id, call_id, method, message };
emplace()? ;-)

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:572: if
(IsBrowserSideNavigationEnabled()) {
nit: where possible, this is better written as

if (!IsBrowserSideNavigationEnabled()) {
   ...
   return;
}

to minimize the diff when cleaning this up later.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:733:
MaybeReattachToRenderFrame();
nit: move into the if above.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:901:
render_frame_alive_ = false;
should we also have frame_host_ = nullptr here?

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:942: return host ==
current || host == pending || host == speculative;
can we add a check for !current_ && !pending_ here?

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.h (right):

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.h:174: int call_id;
I don't think you actually need it.

[pfeldman, 2017-06-10 00:32:05]

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:701:
suspended_messages_.clear();
swap with waiting or extract method DispatchMessageOnRenderer.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:854:
UpdateFrameHost(static_cast<RenderFrameHostImpl*>(new_host));
Try passing null here.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:1088:
frame_tree_node_ = nullptr;
suspended_count = нулю;

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:1192: if
(IsBrowserSideNavigationEnabled() && frame_host_)
nested ifs, return for browser-side early.

[dgozman, 2017-06-10 01:36:35]

PTAL

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:224:
sent_messages_[call_id] = { session_id, call_id, method, message };
On 2017/06/10 00:02:46, caseq wrote:
> emplace()? ;-)

That's more code :(

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:572: if
(IsBrowserSideNavigationEnabled()) {
On 2017/06/10 00:02:46, caseq wrote:
> nit: where possible, this is better written as
> 
> if (!IsBrowserSideNavigationEnabled()) {
>    ...
>    return;
> }
> 
> to minimize the diff when cleaning this up later.

Acknowledged.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:701:
suspended_messages_.clear();
On 2017/06/10 00:32:04, pfeldman wrote:
> swap with waiting or extract method DispatchMessageOnRenderer.

Done.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:733:
MaybeReattachToRenderFrame();
On 2017/06/10 00:02:46, caseq wrote:
> nit: move into the if above.

Done.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:901:
render_frame_alive_ = false;
On 2017/06/10 00:02:46, caseq wrote:
> should we also have frame_host_ = nullptr here?

Nope, this means renderer is dead, but not the host.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:942: return host ==
current || host == pending || host == speculative;
On 2017/06/10 00:02:46, caseq wrote:
> can we add a check for !current_ && !pending_ here?

Done.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:1088:
frame_tree_node_ = nullptr;
On 2017/06/10 00:32:04, pfeldman wrote:
> suspended_count = нулю;

Done.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.cc:1192: if
(IsBrowserSideNavigationEnabled() && frame_host_)
On 2017/06/10 00:32:04, pfeldman wrote:
> nested ifs, return for browser-side early.

Done.

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
File content/browser/devtools/render_frame_devtools_agent_host.h (right):

https://codereview.chromium.org/2927983004/diff/1/content/browser/devtools/re...
content/browser/devtools/render_frame_devtools_agent_host.h:174: int call_id;
On 2017/06/10 00:02:46, caseq wrote:
> I don't think you actually need it.

Done.

[dgozman, 2017-06-10 01:38:48]

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

[commit-bot: I haz the power, 2017-06-10 01:39:04]

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-06-10 01:49:42]

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

[commit-bot: I haz the power, 2017-06-10 01:49:43]

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

[pfeldman, 2017-06-12 17:02:20]

oh boy. lgtm, but please remove logging.

[dgozman, 2017-06-12 17:28:10]

Description was changed from

==========
[DevTools] Rework RFDTAH navigation tracking with browser side navigation

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

to

==========
[DevTools] Rework RFDTAH navigation tracking with browser side navigation

This patch introduces a separate code path for browser side navigation. Instead
of
using pending and current, we keep track of a single RenderFrameHostImpl
instance via
standard WebContentsObserver notifications.

While any navigations are active (determined by the navigation_handles_ set), we
put messages
to suspended_messages_ instead of sending them to the renderer, resuming once
navigations
commit to a specific RenderFrameHostImpl instance.

While navigation is not active, we send messages to renderer and put them to
waiting_for_response_messages_, which are exactly the messages received after
last
state cookie update (see DevToolsMessageChunkProcessor).

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

[dgozman, 2017-06-12 17:50:53]

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

[commit-bot: I haz the power, 2017-06-12 17:51:19]

Dry run: CQ is trying da patch.

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

[dgozman, 2017-06-12 19:47:54]

Thank you for review!

[dgozman, 2017-06-12 20:15:38]

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

[commit-bot: I haz the power, 2017-06-12 20:16:22]

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-06-12 21:27:05]

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

[commit-bot: I haz the power, 2017-06-12 21:27:06]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-06-12 21:34:33]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-06-12 21:34:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2927983004/#ps80001
(title: "removed duplicate policy code")

[commit-bot: I haz the power, 2017-06-12 21:35:04]

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-06-12 21:39:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497303273598150,
"parent_rev": "d5721c7e4a34a0ea63b6b9f78fcc9fdd0d2c967e", "commit_rev":
"7e41e3eccc474549778339d62ff5d55483894f68"}

[commit-bot: I haz the power, 2017-06-12 21:39:18]

Description was changed from

==========
[DevTools] Rework RFDTAH navigation tracking with browser side navigation

This patch introduces a separate code path for browser side navigation. Instead
of
using pending and current, we keep track of a single RenderFrameHostImpl
instance via
standard WebContentsObserver notifications.

While any navigations are active (determined by the navigation_handles_ set), we
put messages
to suspended_messages_ instead of sending them to the renderer, resuming once
navigations
commit to a specific RenderFrameHostImpl instance.

While navigation is not active, we send messages to renderer and put them to
waiting_for_response_messages_, which are exactly the messages received after
last
state cookie update (see DevToolsMessageChunkProcessor).

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

to

==========
[DevTools] Rework RFDTAH navigation tracking with browser side navigation

This patch introduces a separate code path for browser side navigation. Instead
of
using pending and current, we keep track of a single RenderFrameHostImpl
instance via
standard WebContentsObserver notifications.

While any navigations are active (determined by the navigation_handles_ set), we
put messages
to suspended_messages_ instead of sending them to the renderer, resuming once
navigations
commit to a specific RenderFrameHostImpl instance.

While navigation is not active, we send messages to renderer and put them to
waiting_for_response_messages_, which are exactly the messages received after
last
state cookie update (see DevToolsMessageChunkProcessor).

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

Review-Url: https://codereview.chromium.org/2927983004
Cr-Commit-Position: refs/heads/master@{#478774}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e41e3eccc474549778339d62ff5...
==========

[commit-bot: I haz the power, 2017-06-12 21:39:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7e41e3eccc474549778339d62ff5...