Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2191543003:
Remove existing FrameNavigationEntry when new named frame is added. (Closed)
|
Created:
4 years, 4 months ago by nasko Modified:
4 years, 4 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove existing FrameNavigationEntry when new named frame is added.
FrameTreeNode is identified by a unique frame name, which allows us to
look up FrameNavigationEntry objects in NavigationEntry. However, when
a frame is removed from the page, the corresponding FrameNavigationEntry
is not removed. This is done intentionally to support back-forward
navigations in subframes and more intuitive UX on tab restore.
This behavior allows for more than one FrameNavigationEntry to exist in
the NavigationEntry with the same unique name, which can lead to corruption
of session history data. This CL changes the behavior to ensure that
when a new frame is added to the page, any existing entries with the
same unique name are deleted to avoid conflicts.
BUG=628677
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/03ecfad9cb9219e03c87312150aaa5dd89eebc32
Cr-Commit-Position: refs/heads/master@{#409107}
Patch Set 1 #
Total comments: 33
Patch Set 2 : New implementation of FNE clearing. More work needed. #
Total comments: 4
Patch Set 3 : Fixes for all review comments in PS[1-2]. #Patch Set 4 : Rebase to fix conflict #
Total comments: 18
Patch Set 5 : Comments addressed and refactored on top of TreeNode::parent. #
Total comments: 10
Patch Set 6 : Fixes for InSameTreePosition and nits. #Patch Set 7 : Remove NOTREACHED as there are other issues it caught. #
Total comments: 1
Patch Set 8 : Remove extra pointer in test. #
Messages
Total messages: 34 (21 generated)
Description was changed from ========== Remove existing FrameNavigationEntry when new named frame is added. FrameTreeNode is identified by a unique frame name, which allows us to look up FrameNavigationEntry objects in NavigationEntry. However, when a frame is removed from the page, the corresponding FrameNavigationEntry is not removed. This is done intentionally to support back-forward navigations in subframes and more intuitive UX on tab restore. This behavior allows for more than one FrameNavigationEntry to exist in the NavigationEntry with the same unique name, which can lead to corruption of session history data. This CL changes the behavior to ensure that when a new frame is added to the page, any existing entries with the same unique name are deleted to avoid conflicts. BUG=628677 ========== to ========== Remove existing FrameNavigationEntry when new named frame is added. FrameTreeNode is identified by a unique frame name, which allows us to look up FrameNavigationEntry objects in NavigationEntry. However, when a frame is removed from the page, the corresponding FrameNavigationEntry is not removed. This is done intentionally to support back-forward navigations in subframes and more intuitive UX on tab restore. This behavior allows for more than one FrameNavigationEntry to exist in the NavigationEntry with the same unique name, which can lead to corruption of session history data. This CL changes the behavior to ensure that when a new frame is added to the page, any existing entries with the same unique name are deleted to avoid conflicts. BUG=628677 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, I finally have a CL to fix the PageState corruption bug we've been chasing. Can you review it for me? Thanks in advance! Nasko
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Great-- looks like a good start. Haven't looked at the test failures, so we might be missing something. A few other cleanup suggestions below. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree.cc:18: #include "content/browser/frame_host/navigation_entry_impl.h" I suppose we should include NavigationController as well. I'm not thrilled about adding this dependency here, but I suppose there's no good reason to add a hop through NavigatorImpl for this. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree.cc:190: // future updates. Let's clarify a bit how this is an expected case: The last committed NavigationEntry may have a FrameNavigationEntry with the same |frame_unique_name|, since we don't remove FrameNavigationEntries if their frames are deleted. If there is a stale one, remove it to avoid conflicts on future updates. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree.cc:192: root_->navigator()->GetController()->GetLastCommittedEntry()); This should be parent->navigator() to be consistent with line 199 below. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5404: // for UseSubframeNavigationEntries is set to true. Interesting. I've just been making them normal tests and making sure they pass on the Site Isolation Linux/Win FYI bots, which has the advantage that we can verify the test passes in default Chrome as well (possibly skipping any subframe FNE validation if needed). Maybe that's worth doing here? https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5426: EXPECT_EQ(0U, root->child_count()); Can you add an expectation after this that we still have a FrameNavigationEntry for "foo" in the NavigationEntry? (Only in UseSubframeNavigationEntries() mode, if you take my suggestion above.) Would help to mention the reasoning in a comment, like what you have in the CL description. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5435: EXPECT_TRUE(ExecuteScript(root->child_at(0), add_named_frame_script)); // Add a nested frame with the previously used name. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5437: EXPECT_EQ("foo", root->child_at(0)->child_at(0)->frame_name()); Let's verify the tree of FrameNavigationEntries here, too (including the count of children of the root FNE, so that we can tell the old one has been removed). https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5440: EXPECT_EQ(0U, root->child_at(0)->child_count()); I assume this crashed due to your NOTREACHED? https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5449: EnsureFrameNavigationEntriesClearedOnNameConflict) { This seems better documented than the one above, and has a lot of overlap. Does the first one cover a case that this doesn't? I'm wondering if we can get by with just this one. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.cc:796: std::pair<NavigationEntryImpl::TreeNode*, NavigationEntryImpl::TreeNode*>> I suppose that's what we get for not having a parent pointer on TreeNode. Ah well. :) https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.cc:806: if (parent && node->frame_entry->frame_unique_name() == frame_unique_name) { I'm curious if it would be better to use MatchesFrame here, as we do in FindFrameEntry below. Implications: 1) This method would take in a FTN rather than a unique name. 2) We would probably want to call this something like ClearStaleFrameEntries(new_subframe), which might make its purpose clearer. 3) We wouldn't need to check |parent| on this line, since we would never pass in a root FTN. (Maybe we could DCHECK that the FTN has a parent on the first line of this method.) 4) We wouldn't be adding another different way to tell whether a FNE matches (beyond MatchesFrame vs AddOrUpdateFrameEntry). Thoughts? https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.cc:809: if (it != parent->children.end()) { This shouldn't ever fail, should it? https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.h:236: // Removes subframe FrameNavigationEntry matching the |frame_unique_name| and nit: Removes the nit: ... |frame_unique_name|, if any, and... Maybe we should also document that this will remove all such FNEs, but that we expect there to be at most 1 (and will DCHECK otherwise). https://codereview.chromium.org/2191543003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:4481: // since it must come from the same document. Do not update it if difference nit: a difference
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! A few thoughts on PS2 as you go. https://codereview.chromium.org/2191543003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:811: break; It's a little unfortunate that we stop after the first match. Hopefully we'll never see a case with more than one stale entry, but if there were one it'd be hard to track down. Would it be possible to call a helper to do the ancestor walk here, and then continue as you were before? https://codereview.chromium.org/2191543003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:842: current_node = it->second; Need to handle the else branch, right? (If the FTN has a parent and the FNE does not, then the trees don't match.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree.cc:18: #include "content/browser/frame_host/navigation_entry_impl.h" On 2016/07/27 22:48:27, Charlie Reis wrote: > I suppose we should include NavigationController as well. I'm not thrilled > about adding this dependency here, but I suppose there's no good reason to add a > hop through NavigatorImpl for this. Acknowledged. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree.cc:190: // future updates. On 2016/07/27 22:48:27, Charlie Reis wrote: > Let's clarify a bit how this is an expected case: > > The last committed NavigationEntry may have a FrameNavigationEntry with the same > |frame_unique_name|, since we don't remove FrameNavigationEntries if their > frames are deleted. If there is a stale one, remove it to avoid conflicts on > future updates. Done. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree.cc:192: root_->navigator()->GetController()->GetLastCommittedEntry()); On 2016/07/27 22:48:27, Charlie Reis wrote: > This should be parent->navigator() to be consistent with line 199 below. Done. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5404: // for UseSubframeNavigationEntries is set to true. On 2016/07/27 22:48:27, Charlie Reis wrote: > Interesting. I've just been making them normal tests and making sure they pass > on the Site Isolation Linux/Win FYI bots, which has the advantage that we can > verify the test passes in default Chrome as well (possibly skipping any subframe > FNE validation if needed). Maybe that's worth doing here? Sure. Makes sense. I was trying to ensure they run as expected on the default bots. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5426: EXPECT_EQ(0U, root->child_count()); On 2016/07/27 22:48:27, Charlie Reis wrote: > Can you add an expectation after this that we still have a FrameNavigationEntry > for "foo" in the NavigationEntry? (Only in UseSubframeNavigationEntries() mode, > if you take my suggestion above.) Would help to mention the reasoning in a > comment, like what you have in the CL description. Done. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5435: EXPECT_TRUE(ExecuteScript(root->child_at(0), add_named_frame_script)); On 2016/07/27 22:48:27, Charlie Reis wrote: > // Add a nested frame with the previously used name. Done. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5437: EXPECT_EQ("foo", root->child_at(0)->child_at(0)->frame_name()); On 2016/07/27 22:48:27, Charlie Reis wrote: > Let's verify the tree of FrameNavigationEntries here, too (including the count > of children of the root FNE, so that we can tell the old one has been removed). Done. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5440: EXPECT_EQ(0U, root->child_at(0)->child_count()); On 2016/07/27 22:48:27, Charlie Reis wrote: > I assume this crashed due to your NOTREACHED? Indeed! https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5449: EnsureFrameNavigationEntriesClearedOnNameConflict) { On 2016/07/27 22:48:27, Charlie Reis wrote: > This seems better documented than the one above, and has a lot of overlap. Does > the first one cover a case that this doesn't? I'm wondering if we can get by > with just this one. This one actually navigates the iframes to real URLs whereas the other test leaves them at about:blank. If you think this is a difference that isn't significant, I'm ok with removing the other test case. I was just thinking that initial empty document might have different behaviors, but they both should emit a DidCommitProvisionalLoad IPC, so maybe the difference is not material. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.cc:796: std::pair<NavigationEntryImpl::TreeNode*, NavigationEntryImpl::TreeNode*>> On 2016/07/27 22:48:28, Charlie Reis wrote: > I suppose that's what we get for not having a parent pointer on TreeNode. Ah > well. :) Acknowledged. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.cc:806: if (parent && node->frame_entry->frame_unique_name() == frame_unique_name) { On 2016/07/27 22:48:28, Charlie Reis wrote: > I'm curious if it would be better to use MatchesFrame here, as we do in > FindFrameEntry below. Implications: > 1) This method would take in a FTN rather than a unique name. > 2) We would probably want to call this something like > ClearStaleFrameEntries(new_subframe), which might make its purpose clearer. > 3) We wouldn't need to check |parent| on this line, since we would never pass in > a root FTN. (Maybe we could DCHECK that the FTN has a parent on the first line > of this method.) > 4) We wouldn't be adding another different way to tell whether a FNE matches > (beyond MatchesFrame vs AddOrUpdateFrameEntry). > > Thoughts? Done. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.cc:809: if (it != parent->children.end()) { On 2016/07/27 22:48:27, Charlie Reis wrote: > This shouldn't ever fail, should it? No. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.h:236: // Removes subframe FrameNavigationEntry matching the |frame_unique_name| and On 2016/07/27 22:48:28, Charlie Reis wrote: > nit: Removes the > nit: ... |frame_unique_name|, if any, and... > > Maybe we should also document that this will remove all such FNEs, but that we > expect there to be at most 1 (and will DCHECK otherwise). What does documenting this here give us? It will be adding description of what bugs we might have, which the DCHECK is aimed at catching in the first place. https://codereview.chromium.org/2191543003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:4481: // since it must come from the same document. Do not update it if difference On 2016/07/27 22:48:28, Charlie Reis wrote: > nit: a difference Done. https://codereview.chromium.org/2191543003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:811: break; On 2016/07/28 22:57:01, Charlie Reis wrote: > It's a little unfortunate that we stop after the first match. Hopefully we'll > never see a case with more than one stale entry, but if there were one it'd be > hard to track down. > > Would it be possible to call a helper to do the ancestor walk here, and then > continue as you were before? Done. https://codereview.chromium.org/2191543003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:842: current_node = it->second; On 2016/07/28 22:57:01, Charlie Reis wrote: > Need to handle the else branch, right? (If the FTN has a parent and the FNE > does not, then the trees don't match.) Done.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! It's always fun trying to shuffle the code around until we find something that fits well. I think we're closing in on something nice. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5449: EnsureFrameNavigationEntriesClearedOnNameConflict) { On 2016/07/29 15:52:20, nasko wrote: > On 2016/07/27 22:48:27, Charlie Reis wrote: > > This seems better documented than the one above, and has a lot of overlap. > Does > > the first one cover a case that this doesn't? I'm wondering if we can get by > > with just this one. > > This one actually navigates the iframes to real URLs whereas the other test > leaves them at about:blank. If you think this is a difference that isn't > significant, I'm ok with removing the other test case. I was just thinking that > initial empty document might have different behaviors, but they both should emit > a DidCommitProvisionalLoad IPC, so maybe the difference is not material. Ah, that didn't come across from the comments or test names. Since this one is better documented, I'd suggest listing it first, and then describe the other one as similar but with no src URLs. Now that you mention it, there's actually 3 cases: real URL, no-src, and slow URL. The first two behave more or less the same, in that we normally get a commit and won't get an UpdateState until after that. I'm ok with having separate tests for them. In the slow URL case, we could see an UpdateState before the first commit (e.g., if we put text into a form input and then wait a second). Before your fix, that UpdateState could end up on a stale FNE, and now it should be fixed. I wonder if it's worth having a test for that case as well? The tricky part is waiting long enough for the UpdateState message to arrive, since I don't think we have anything to wait for. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.h:236: // Removes subframe FrameNavigationEntry matching the |frame_unique_name| and On 2016/07/29 15:52:22, nasko wrote: > On 2016/07/27 22:48:28, Charlie Reis wrote: > > nit: Removes the > > nit: ... |frame_unique_name|, if any, and... > > > > Maybe we should also document that this will remove all such FNEs, but that we > > expect there to be at most 1 (and will DCHECK otherwise). > > What does documenting this here give us? It will be adding description of what > bugs we might have, which the DCHECK is aimed at catching in the first place. I think it matters for potential callers of the method. For example, this method would not guarantee the current bug is fixed if it were implemented as described (i.e., it stopped after the first one), because there could still be more colliding FNEs in the tree. We don't need to mention the DCHECK, but I think we can still clarify: Removes any subframe FrameNavigationEntries that match the unique name of |frame_tree_node|, and all of their children. We expect there to be at most one, since we avoid collisions but leave old FrameNavigationEntries in the tree after their frame has been detached. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:204: last_committed_entry->ClearMatchingFrameEntries(added_node); Maybe the name can be clearer about the fact that such entries would be stale or collisions? What about ClearStaleFrameEntriesForNewFrame? https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:102: std::map<NavigationEntryImpl::TreeNode*, NavigationEntryImpl::TreeNode*>; nit: Please document which parameter is which. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:105: // |node|. If a mismatch in unique name is detected, |node| needs to be removed. nit: There's no |node| parameter. :) It's also unclear what to pass in here. I think frame_tree_node is supposed to be the match for parent_node, but that's not obvious without looking at the implementation. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:106: void WalkAncestorsAndRemoveIfMismatchFound( Can we make this just return whether the ancestors match, and let the other function do the removal? Such a helper would be simpler and might be useful in other contexts. Something like DoAncestorsMatch(frame_tree_node, tree_node, parenting_map, root_node)? Or InSameTreePosition? (That order of params might also make it clearer to pass: the first two are the ones being compared directly, and the rest are the necessary context.) https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:115: if (!current_node->MatchesFrame(ftn, current_node == root_node)) { I apologize for MatchesFrame taking in is_root_tree_node. :( That's the only reason we need to pass |root_node| here, right? Hopefully we can remove that if we prevent frames from changing their unique name after creation time. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:118: DCHECK(it != parent_node->children.end()); nit: DCHECK_NE, unless the compiler doesn't like it. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:120: return; Both of these erase blocks could just be return false, eliminating the code duplication. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:123: auto parent_iterator = parenting_map.find(current_node); Do we still need parenting_map as opposed to having parent pointers? I'm trying to understand why a parent pointer would be insufficient now. (Bonus points: Having a parent pointer would mean we don't need is_root_tree_node in MatchesFrame!) https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:873: DCHECK_GE(1, count); I think this might be easier to read as DCHECK_LE(count, 1), even though they're functionally equivalent. I was reading the current one as "make sure we delete at least 1" instead of "at most 1."
Another round ready to go. There are few tests that fail in --site-per-process or PlzNavigate mode, which I'll investigate in the meantime. However, just enabling UseSubframeNavigationEntries on its own should be green. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5449: EnsureFrameNavigationEntriesClearedOnNameConflict) { On 2016/07/29 17:21:35, Charlie Reis wrote: > On 2016/07/29 15:52:20, nasko wrote: > > On 2016/07/27 22:48:27, Charlie Reis wrote: > > > This seems better documented than the one above, and has a lot of overlap. > > Does > > > the first one cover a case that this doesn't? I'm wondering if we can get > by > > > with just this one. > > > > This one actually navigates the iframes to real URLs whereas the other test > > leaves them at about:blank. If you think this is a difference that isn't > > significant, I'm ok with removing the other test case. I was just thinking > that > > initial empty document might have different behaviors, but they both should > emit > > a DidCommitProvisionalLoad IPC, so maybe the difference is not material. > > Ah, that didn't come across from the comments or test names. Since this one is > better documented, I'd suggest listing it first, and then describe the other one > as similar but with no src URLs. Reordered and renamed to match a bit better. > Now that you mention it, there's actually 3 cases: real URL, no-src, and slow > URL. The first two behave more or less the same, in that we normally get a > commit and won't get an UpdateState until after that. I'm ok with having > separate tests for them. In the slow URL case, we could see an UpdateState > before the first commit (e.g., if we put text into a form input and then wait a > second). Before your fix, that UpdateState could end up on a stale FNE, and now > it should be fixed. > > I wonder if it's worth having a test for that case as well? The tricky part is > waiting long enough for the UpdateState message to arrive, since I don't think > we have anything to wait for. I can try to write such a test. Maybe using a BrowserMessageFilter can help, since I will be able to signal to the test when the UpdateState IPC has been received. Are you ok with it being in a separate CL so we can have this landed without waiting for that test? https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_entry_impl.h:236: // Removes subframe FrameNavigationEntry matching the |frame_unique_name| and On 2016/07/29 17:21:35, Charlie Reis wrote: > On 2016/07/29 15:52:22, nasko wrote: > > On 2016/07/27 22:48:28, Charlie Reis wrote: > > > nit: Removes the > > > nit: ... |frame_unique_name|, if any, and... > > > > > > Maybe we should also document that this will remove all such FNEs, but that > we > > > expect there to be at most 1 (and will DCHECK otherwise). > > > > What does documenting this here give us? It will be adding description of what > > bugs we might have, which the DCHECK is aimed at catching in the first place. > > I think it matters for potential callers of the method. For example, this > method would not guarantee the current bug is fixed if it were implemented as > described (i.e., it stopped after the first one), because there could still be > more colliding FNEs in the tree. > > We don't need to mention the DCHECK, but I think we can still clarify: > > Removes any subframe FrameNavigationEntries that match the unique name of > |frame_tree_node|, and all of their children. We expect there to be at most > one, since we avoid collisions but leave old FrameNavigationEntries in the tree > after their frame has been detached. Done. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:204: last_committed_entry->ClearMatchingFrameEntries(added_node); On 2016/07/29 17:21:35, Charlie Reis wrote: > Maybe the name can be clearer about the fact that such entries would be stale or > collisions? What about ClearStaleFrameEntriesForNewFrame? Done. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:102: std::map<NavigationEntryImpl::TreeNode*, NavigationEntryImpl::TreeNode*>; On 2016/07/29 17:21:36, Charlie Reis wrote: > nit: Please document which parameter is which. Removed in refactor. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:105: // |node|. If a mismatch in unique name is detected, |node| needs to be removed. On 2016/07/29 17:21:35, Charlie Reis wrote: > nit: There's no |node| parameter. :) > > It's also unclear what to pass in here. I think frame_tree_node is supposed to > be the match for parent_node, but that's not obvious without looking at the > implementation. Done. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:106: void WalkAncestorsAndRemoveIfMismatchFound( On 2016/07/29 17:21:36, Charlie Reis wrote: > Can we make this just return whether the ancestors match, and let the other > function do the removal? Such a helper would be simpler and might be useful in > other contexts. Something like DoAncestorsMatch(frame_tree_node, tree_node, > parenting_map, root_node)? Or InSameTreePosition? (That order of params might > also make it clearer to pass: the first two are the ones being compared > directly, and the rest are the necessary context.) Done. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:115: if (!current_node->MatchesFrame(ftn, current_node == root_node)) { On 2016/07/29 17:21:36, Charlie Reis wrote: > I apologize for MatchesFrame taking in is_root_tree_node. :( That's the only > reason we need to pass |root_node| here, right? Hopefully we can remove that if > we prevent frames from changing their unique name after creation time. Done. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:118: DCHECK(it != parent_node->children.end()); On 2016/07/29 17:21:35, Charlie Reis wrote: > nit: DCHECK_NE, unless the compiler doesn't like it. Compiler complains about it. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:120: return; On 2016/07/29 17:21:36, Charlie Reis wrote: > Both of these erase blocks could just be return false, eliminating the code > duplication. Done. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:123: auto parent_iterator = parenting_map.find(current_node); On 2016/07/29 17:21:35, Charlie Reis wrote: > Do we still need parenting_map as opposed to having parent pointers? I'm trying > to understand why a parent pointer would be insufficient now. > > (Bonus points: Having a parent pointer would mean we don't need > is_root_tree_node in MatchesFrame!) Done. https://codereview.chromium.org/2191543003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:873: DCHECK_GE(1, count); On 2016/07/29 17:21:35, Charlie Reis wrote: > I think this might be easier to read as DCHECK_LE(count, 1), even though they're > functionally equivalent. I was reading the current one as "make sure we delete > at least 1" instead of "at most 1." Done.
Great. A few nits and one possible bug below. https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5449: EnsureFrameNavigationEntriesClearedOnNameConflict) { On 2016/08/01 20:42:15, nasko wrote: > On 2016/07/29 17:21:35, Charlie Reis wrote: > > On 2016/07/29 15:52:20, nasko wrote: > > > On 2016/07/27 22:48:27, Charlie Reis wrote: > > > > This seems better documented than the one above, and has a lot of overlap. > > > > Does > > > > the first one cover a case that this doesn't? I'm wondering if we can get > > by > > > > with just this one. > > > > > > This one actually navigates the iframes to real URLs whereas the other test > > > leaves them at about:blank. If you think this is a difference that isn't > > > significant, I'm ok with removing the other test case. I was just thinking > > that > > > initial empty document might have different behaviors, but they both should > > emit > > > a DidCommitProvisionalLoad IPC, so maybe the difference is not material. > > > > Ah, that didn't come across from the comments or test names. Since this one > is > > better documented, I'd suggest listing it first, and then describe the other > one > > as similar but with no src URLs. > > Reordered and renamed to match a bit better. > > > Now that you mention it, there's actually 3 cases: real URL, no-src, and slow > > URL. The first two behave more or less the same, in that we normally get a > > commit and won't get an UpdateState until after that. I'm ok with having > > separate tests for them. In the slow URL case, we could see an UpdateState > > before the first commit (e.g., if we put text into a form input and then wait > a > > second). Before your fix, that UpdateState could end up on a stale FNE, and > now > > it should be fixed. > > > > I wonder if it's worth having a test for that case as well? The tricky part > is > > waiting long enough for the UpdateState message to arrive, since I don't think > > we have anything to wait for. > > I can try to write such a test. Maybe using a BrowserMessageFilter can help, > since I will be able to signal to the test when the UpdateState IPC has been > received. Are you ok with it being in a separate CL so we can have this landed > without waiting for that test? Yes. I'm curious how that works out, since I'd love to use such a thing in a few other tests. https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5508: // name. It is similar to EnsureFrameNavigationEntriesClearedOnConflict, but EnsureFrameNavigationEntriesClearedOnMismatch https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:107: FrameTreeNode* frame_tree_node, Style nit: I think this will fit on the previous line, with the next line indented accordingly. https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:115: if (!current_node->parent) Is this a bug? Seems like if you call this for a first-level subframe, we'll compare the main frame's FTN and TreeNode in the first iteration of the loop. They'll match on line 112, but this will make us return false. Seems like we only want to return false if one has a parent and the other doesn't. (If that's right, can you make sure we have test coverage for it?) https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:857: // Remove the node from the tree if it is not for in the same position in nit: Drop "for" https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:863: DCHECK(it != parent_node->children.end()); Let's upgrade this to a CHECK. (Otherwise, could it be a memory error below?)
https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5508: // name. It is similar to EnsureFrameNavigationEntriesClearedOnConflict, but On 2016/08/01 21:04:25, Charlie Reis wrote: > EnsureFrameNavigationEntriesClearedOnMismatch Done. https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.cc (right): https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:107: FrameTreeNode* frame_tree_node, On 2016/08/01 21:04:25, Charlie Reis wrote: > Style nit: I think this will fit on the previous line, with the next line > indented accordingly. I wonder why git cl format didn't pick it up initially. Fixed. https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:115: if (!current_node->parent) On 2016/08/01 21:04:25, Charlie Reis wrote: > Is this a bug? Seems like if you call this for a first-level subframe, we'll > compare the main frame's FTN and TreeNode in the first iteration of the loop. > They'll match on line 112, but this will make us return false. > > Seems like we only want to return false if one has a parent and the other > doesn't. > > (If that's right, can you make sure we have test coverage for it?) Done. https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:857: // Remove the node from the tree if it is not for in the same position in On 2016/08/01 21:04:25, Charlie Reis wrote: > nit: Drop "for" Done. https://codereview.chromium.org/2191543003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.cc:863: DCHECK(it != parent_node->children.end()); On 2016/08/01 21:04:25, Charlie Reis wrote: > Let's upgrade this to a CHECK. (Otherwise, could it be a memory error below?) Done.
Ready for another round of reviews. Just the NOTREACHED removed in the latest patchset.
Thanks! LGTM. I'm ok with delaying the NOTREACHED until we fix the replace case. https://codereview.chromium.org/2191543003/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2191543003/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:5584: FrameTreeNode* node = root; nit: No need for this.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2191543003/#ps140001 (title: "Remove extra pointer in test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Remove existing FrameNavigationEntry when new named frame is added. FrameTreeNode is identified by a unique frame name, which allows us to look up FrameNavigationEntry objects in NavigationEntry. However, when a frame is removed from the page, the corresponding FrameNavigationEntry is not removed. This is done intentionally to support back-forward navigations in subframes and more intuitive UX on tab restore. This behavior allows for more than one FrameNavigationEntry to exist in the NavigationEntry with the same unique name, which can lead to corruption of session history data. This CL changes the behavior to ensure that when a new frame is added to the page, any existing entries with the same unique name are deleted to avoid conflicts. BUG=628677 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove existing FrameNavigationEntry when new named frame is added. FrameTreeNode is identified by a unique frame name, which allows us to look up FrameNavigationEntry objects in NavigationEntry. However, when a frame is removed from the page, the corresponding FrameNavigationEntry is not removed. This is done intentionally to support back-forward navigations in subframes and more intuitive UX on tab restore. This behavior allows for more than one FrameNavigationEntry to exist in the NavigationEntry with the same unique name, which can lead to corruption of session history data. This CL changes the behavior to ensure that when a new frame is added to the page, any existing entries with the same unique name are deleted to avoid conflicts. BUG=628677 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/03ecfad9cb9219e03c87312150aaa5dd89eebc32 Cr-Commit-Position: refs/heads/master@{#409107} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/03ecfad9cb9219e03c87312150aaa5dd89eebc32 Cr-Commit-Position: refs/heads/master@{#409107} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
