Video & Bugreport attached =>  http://go/chrome-androidlogs1/8/818025

Thanks for filing a bug for this, this is exactly what I observed when investigating issue 813739 (see bullet #2 in comment #8).

This is specific to multiprocess (and it repros on N with multiprocess enabled). Basic issue is related to the Set() and Erase() methods for frame_tree_node_to_io_thread_client_ (see aw_contents_io_thread_client.cc).

We Erase() ID 1 (main frame) from the map, but don't Set() it again. So when we try to check that resource, it says that Safe Browsing is not enabled for that IoThreadClientData object (because we can't find it in our map). Interestingly, subresources still work fine (so clicking the "malicious subresource" link will trigger the interstitial as expected).

So the question remains: are we missing a call to Set(id = 1) after the final Erase(id = 1), or do we have one too many Erase(id = 1) calls? I'll do more investigation.

Also, this means that multiprocess effectively disables Safe Browsing, which means that multiprocess paradoxically reduces security :(

Huh, turns out this is a regression from PlzNavigate. Range:

https://chromium.googlesource.com/chromium/src/+log/63.0.3215.0..63.0.3217.0?pretty=fuller

Confirmed that the "--enable-browser-side-navigation" switch breaks this for early M63 builds and the "--disable-browser-side-navigation" switch fixes it for later PlzNavigate builds.

Follow-up to the investigation in comment #2. It looks like pre-plznavigate we never actually check frame_tree_node_id's, so the pattern of Set/Erase doesn't appear to affect Safe Browsing. Still unsure of the correct behavior.

Chatted with Bo today. We think our map is incorrect.

Our 2 maps [1] are:

 1. RenderFrameHost -> IoThreadClientData
 2. FrameTreeNode -> IoThreadClientData (added for PlzNavigate)

The basic issue is that we Set & Erase both maps in similar spots, but FrameTreeNode and RenderFrameHost don't have a 1:1 relationship (a FrameTreeNode may have multiple RenderFrameHosts).

I did 3 cross-site browser-initiated navigations. The Set and Erase calls look like:

Set   FrameTreeNode 1 / RenderFrameHost (2,2) (this happens when the WebView is instantiated)
Set   FrameTreeNode 1 / RenderFrameHost (2,2) (this is the first navigation)

Set   FrameTreeNode 1 / RenderFrameHost (2,5)
Erase FrameTreeNode 1 / RenderFrameHost (2,2) <-- problem starts here: map 2 is empty.

Set   FrameTreeNode 1 / RenderFrameHost (2,9)
Erase FrameTreeNode 1 / RenderFrameHost (2,5)

---

Root cause: the maps fall out of sync because map 2 doesn't distinguish between RenderFrameHosts for a given FrameTreeNode. If we later do a renderer-initiated navigation, we try to check for FrameTreeNode 1 in the map, but we already erased it.

jam@ do you know if map 1 and map 2 are designed to stay in-sync? I see you filed  issue 645983 , where we originally introduced map 2.

[1] https://cs.chromium.org/chromium/src/android_webview/browser/aw_contents_io_thread_client.cc?type=cs&sq=package:chromium&l=84

The maps should always hold the correct state of the world, and the FrameTreeNode does not, so is broken should be fixed.

If both maps should hold the state of the world, then we should consolidate them into one.

huh? but they are different maps, with different keys..

Both maps are implementation details of AwContentsIoThreadClient::FromID() (there are 2 methods with different signatures). Judging by the name, it sounds wrong for one FromID(frame_tree_node_id) to return one thing and FromID(render_process_id, render_frame_id) to return something else.

Since both FromID() methods serve a similar purpose, and should return equivalent* values given valid input, then it makes sense for them to use the same internal state.

* FromID() actually returns unique_ptr<AwContentsIoThreadClient>, so invoking each method would return different unique pointers, however the underlying AwContentsIoThreadClient native object should pin the same AwContentsIoThreadClient Java object [1]. So these don't return "equal" values, but return values are arguably conceptually "equivalent" because they refer to the same Java instance.

[1] https://cs.chromium.org/chromium/src/android_webview/browser/aw_contents_io_thread_client.h?type=cs&sq=package:chromium&l=42

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1715b8dc26a9b994de4fcb3487b0bb98f16fba2e

commit 1715b8dc26a9b994de4fcb3487b0bb98f16fba2e
Author: Nate Fischer <ntfschr@chromium.org>
Date: Sat Mar 10 03:03:29 2018

AW: fix AwContentsIoThreadClient::FromID map

This fixes a bug related to the implementation of
AwContentsIoThreadClient::FromID(frame_tree_node_id). This function is
implemented using a map to keep track of the AwContentsIoThreadClient
for a particular FrameTreeNode.

Before this change, we would remove a map entry for a frame_tree_node_id
whenever we destroy an associated RenderFrameHost. This is incorrect in
multiprocess mode however, because a FrameTreeNode may have multiple
associated RenderFrameHosts in its lifetime (such as swapping
RenderFrameHosts for cross-site navigations). When we swap, we would
first overwrite the map entry for the new RenderFrameHost, then erase
the map entry when we destroy the old RenderFrameHost.

The issue manifests most visibly for Safe Browsing, because FromID()
returns nullptr, and Safe Browsing logic defaults to disabling the
feature. However, this issue is not specific to Safe Browsing, and this
would bug would cause a bad state whenever we trigger multiple
cross-site browser-initiated navigations.

With this change, we keep track of live RenderFrameHosts for a given
FrameTreeNode, and only remove an entry from the map if we've destroyed
the last associated RenderFrameHost.

Bug:  818025 
Test: manual - see bug for repro steps
Change-Id: I2ae81fdb5338084cdf253582206144b4f45704b1
Reviewed-on: https://chromium-review.googlesource.com/956411
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542334}
[modify] https://crrev.com/1715b8dc26a9b994de4fcb3487b0bb98f16fba2e/android_webview/browser/aw_contents_io_thread_client.cc

@Nate : Can you please merge the fix to M66 branch?. I could verify the fix on Tot.Thanks

Reopening this to request a merge. This landed in Monday's canary (67.0.3368.0). As per the 24-hr requirement, I'll hold until Tuesday for the merge.

In terms of risk, this will have no impact on Chrome.

Merge approved upon verification of the fix in canary

Please merge this change as soon as possible

this issue no longer reproducible on 67.0.3370.0 (ToT) . Please merge the changes to M66 .Thanks

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a262cc22e17455c7ec69690ae70be74ffc745b84

commit a262cc22e17455c7ec69690ae70be74ffc745b84
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Mar 15 03:25:16 2018

AW: fix AwContentsIoThreadClient::FromID map

This fixes a bug related to the implementation of
AwContentsIoThreadClient::FromID(frame_tree_node_id). This function is
implemented using a map to keep track of the AwContentsIoThreadClient
for a particular FrameTreeNode.

Before this change, we would remove a map entry for a frame_tree_node_id
whenever we destroy an associated RenderFrameHost. This is incorrect in
multiprocess mode however, because a FrameTreeNode may have multiple
associated RenderFrameHosts in its lifetime (such as swapping
RenderFrameHosts for cross-site navigations). When we swap, we would
first overwrite the map entry for the new RenderFrameHost, then erase
the map entry when we destroy the old RenderFrameHost.

The issue manifests most visibly for Safe Browsing, because FromID()
returns nullptr, and Safe Browsing logic defaults to disabling the
feature. However, this issue is not specific to Safe Browsing, and this
would bug would cause a bad state whenever we trigger multiple
cross-site browser-initiated navigations.

With this change, we keep track of live RenderFrameHosts for a given
FrameTreeNode, and only remove an entry from the map if we've destroyed
the last associated RenderFrameHost.

Bug:  818025 
Test: manual - see bug for repro steps
Change-Id: I2ae81fdb5338084cdf253582206144b4f45704b1
Reviewed-on: https://chromium-review.googlesource.com/956411
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#542334}(cherry picked from commit 1715b8dc26a9b994de4fcb3487b0bb98f16fba2e)
Reviewed-on: https://chromium-review.googlesource.com/963676
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#263}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/a262cc22e17455c7ec69690ae70be74ffc745b84/android_webview/browser/aw_contents_io_thread_client.cc

Verified on Nexus 6P/OPM1.171019.028 having 65.0.3359.35 and 67.0.3372.0 , issue mentioned in #0 no longer reproducible. Thanks for the fix and merge to 66.

Opening this up, because there's nothing private.

 Issue 830850  has been merged into this issue.