New issue
Advanced search Search tips

Issue 774637 link

Starred by 1 user
Status: Assigned
Owner:
creis@chromium.org
Cc:
lukasza@chromium.org
dcheng@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Use same FNE lookup technique for GetFrameEntry and AddOrUpdateFrameEntry

Project Member Reported by creis@chromium.org, Oct 13 2017 
As found in  issue 766262 , NavigationController can end up modifying two different FrameNavigationEntries at commit time, because there are two different algorithms used for looking them up.

AddOrUpdateFrameEntry first finds the parent frame's FNE, then looks for the target frame among the immediate children.  GetFrameEntry (used later for ResetForCommit and a few other operations) instead looks up the target frame purely by unique name, without first finding the parent.

If there is more than one instance of a unique name in the tree (since we don't currently verify uniqueness on the browser side and have a race in issue 616820), then these two algorithms can yield different FNEs.  Specifically, this can happen for a page like:

   A
  / \
 B   C
     |
     B

If the child frame of C commits a navigation, AddOrUpdateFrameEntry(B) will return the correct frame (child of C), but GetFrameEntry(B) will return the first child of A.

We should use a consistent approach to make sure we don't incompletely update a FNE.  (This is in addition to fixing ways that we can end up with unique name collisions.)

Comment 1 by lukasza@chromium.org, Oct 13 2017 

What should happen when there are 2 matches for a given unique name?

- Pick an arbitrary FNE? (this is the behavior today;  this bug seems to focus on the arbitrariness being consistent everywhere)

- Modify all the matching FNEs?

- Modify no FNEs?


Maybe making no modifications in case of a conflict is the safest approach?

Comment 2 by creis@chromium.org, Oct 13 2017 

I think we should pick a consistent, arbitrary one, making at least some effort to tell if it's the right one.  (We could check that the FTN ancestor chain matches the FNE ancestor chain, for example, and not just look up the parent first.)

It's fairly bad to update no FNEs after a commit, which is why I'd like to avoid it.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 13 2017 

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

commit f4448202d63cc64d703637305921239ff789e280
Author: Charles Reis <creis@chromium.org>
Date: Fri Oct 13 21:15:03 2017

Set PageState and redirects consistently for FrameNavigationEntries.

Move the update of PageState within each RendererDidNavigate* helper
method, so that it's done on the correct frame.  Due to issue 774637,
there are cases where the PageState was being applied to the wrong
FNE in the past.

BUG= 766262 , 774637

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I78125efdc32d80795ef58afa65be28d374a1366c
Reviewed-on: https://chromium-review.googlesource.com/718080
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508821}
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/frame_navigation_entry.cc
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/frame_navigation_entry.h
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/navigation_entry_impl.cc
[modify] https://crrev.com/f4448202d63cc64d703637305921239ff789e280/content/browser/frame_host/navigator_impl.cc
►

Sign in to add a comment