• arthursonzogni's avatar
    DevTools: Be more reliable when the navigation commits after RFH swap. · b1fc15a9
    arthursonzogni authored
    The main motivation behind this CL is to land:
    https://chromium-review.googlesource.com/c/chromium/src/+/1688957
    It makes every navigations from a crashed RenderFrameHost to use a new
    RenderFrameHost. Previously, only the cross-origin navigations where using a
    new RenderFrameHost.
    
    DevToolsProtocolTest was only used with same-process navigation, this CL adds
    a regression test:
    DevToolsProtocolTest.InspectorTargetCrashedNavigateCrossProcess
    This test fails without this patch and works with it.
    
    Context:
    
    RenderFrameDevToolsAgentHost follows the current document in a given frame.
    This translates by following a RenderFrameHost in a given FrameTreeNode.
    
    To do this, most observers of a WebContents uses:
    WebContentsObserver::RenderFrameHostChanged(old_rfh, new_rfh). They updates the
    current RenderFrameHost after the navigation has been done and the old and new
    RenderFrameHost have been swapped.
    
    The RenderFrameDevToolsAgenHost way of doing things is slightly different, it
    wants to follow the new RenderFrameHost as soon as possible. It happens when the
    navigation is ReadyToCommit().
    
    To handle canceled navigation and crashed RenderFrameHost, the
    RenderFrameDevToolsAgentHost observes the following "events":
     * ReadyToCommitNavigation
     * DidFinishNavigation
     * RenderFrameHostChanged.
    Please take a look at their implementations.
    
    Depending on the case, they are called in a different order and produces
    different results.
    
    Let's explore several examples with a cross-process navigation from A to B:
    
    ────────────────────────────────────────────────────────────────────────────────
    
    Case 1: Successful navigation from a living RenderFrameHost.
      ┌─┬────────────────────────┬───────────────────┬───────────────────┐
      │#│ Event                  │ before patch      │ after patch       │
      ├─┼────────────────────────┼───────────────────┼───────────────────┤
      │1│ ReadyToCommitNavigation│ UpdateFrameHost(B)│ UpdateFrameHost(B)│
      ├─┼────────────────────────┼───────────────────┼───────────────────┤
      │2│ RenderFrameHostChanged │                   │ UpdateFrameHost(B)│
      ├─┼────────────────────────┼───────────────────┼───────────────────┤
      │3│ DidFinishNavigation    │ UpdateFrameHost(B)│ UpdateFrameHost(B)│
      └─┴────────────────────────┴───────────────────┴───────────────────┘
    
      Observation: This produces the same effects, UpdateFrameHost called several
      time in a row with the same RenderFrameHost is essentially a no-op.
    
    ────────────────────────────────────────────────────────────────────────────────
    
    Case 2: Successful navigation from a dead RenderFrameHost.
      ┌─┬────────────────────────┬─────────────────────────┬───────────────────┐
      │#│ Event                  │ before patch            │ after patch       │
      ├─┼────────────────────────┼─────────────────────────┼───────────────────┤
      │1│ RenderFrameHostChanged │ UpdateFrameHost(nullptr)│ UpdateFrameHost(B)│
      ├─┼────────────────────────┼─────────────────────────┼───────────────────┤
      │2│ ReadyToCommitNavigation│ UpdateFrameHost(B)      │ UpdateFrameHost(B)│
      ├─┼────────────────────────┼─────────────────────────┼───────────────────┤
      │3│ DidFinishNavigation    │ UpdateFrameHost(B)      │ UpdateFrameHost(B)│
      └─┴────────────────────────┴─────────────────────────┴───────────────────┘
    
      Observation: Without this patch, when the new RenderFrameHost starts loading,
      it temporarily switch to nullptr for a short time. It is no more the case.
    
    ────────────────────────────────────────────────────────────────────────────────
    
    Case 3: Navigation canceled after ReadyToCommitNavigation.
      ┌─┬────────────────────────┬───────────────────┬───────────────────┐
      │#│ Event                  │ before patch      │ after patch       │
      ├─┼────────────────────────┼───────────────────┼───────────────────┤
      │1│ ReadyToCommitNavigation│ UpdateFrameHost(B)│ UpdateFrameHost(B)│
      ├─┼────────────────────────┼───────────────────┼───────────────────┤
      │2│ DidFinishNavigation    │ UpdateFrameHost(A)│ UpdateFrameHost(A)│
      └─┴────────────────────────┴───────────────────┴───────────────────┘
    
      Observation: Nothing changed.
    
    ────────────────────────────────────────────────────────────────────────────────
    
    The previous way of "following" RenderFrameHost by the
    RenderFrameDevToolsAgentHost can "temporary" follow nullptr in case 2. This is
    no more the case.
    
    Bug: 990315
    Change-Id: I0703e1602ef47f8964a5041cc509ed0344bf4e55
    Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1731489Reviewed-by: default avatarAlex Moshchuk <alexmos@chromium.org>
    Reviewed-by: default avatarAndrey Kosyakov <caseq@chromium.org>
    Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#684259}
    b1fc15a9
render_frame_devtools_agent_host.cc 29.2 KB