|
|
Created:
3 years, 8 months ago by arthursonzogni Modified:
3 years, 8 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, clamy, nasko Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL makes that the following invariant stay true:
If (pending_entry_index != -1)
Then (pending_entry_ = entries[pending_entry_index_])
There were several parts of the code that make it become false.
The bug was discovered thanks to
[Add DCHECKs for invariants on NavigationController indices.]
(https://codereview.chromium.org/2815753003/)
BUG=709431
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
Review-Url: https://codereview.chromium.org/2810173002
Cr-Commit-Position: refs/heads/master@{#467164}
Committed: https://chromium.googlesource.com/chromium/src/+/5c4c202d728cacf71c4ac518e9ab5ee8440eafcf
Patch Set 1 : git cl try with invariant DCHECK #Patch Set 2 : Additionnal fixes #Patch Set 3 : move/delete DCHECK #Patch Set 4 : One more fix. #Patch Set 5 : Remove DCHECK because of WebContentsImplTest.ShowInterstitialThenNavigate. #
Total comments: 11
Patch Set 6 : Addressed comments @creis #
Total comments: 6
Patch Set 7 : Addressed comments @creis #2 #
Messages
Total messages: 89 (75 generated)
Description was changed from ========== NavigationController: Fix pending_entry_ pointer not updated. In the NavigationControllerImpl the pending_entry_index_ was updated without the pending_entry_. This breaks invariants in the code. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 ========== to ========== NavigationController: Fix pending_entry_ pointer not updated. In the NavigationControllerImpl the pending_entry_index_ was updated without the pending_entry_. This breaks invariants in the code. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== NavigationController: Fix pending_entry_ pointer not updated. In the NavigationControllerImpl the pending_entry_index_ was updated without the pending_entry_. This breaks invariants in the code. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== NavigationController: Fix pending_entry_ pointer not updated. In the NavigationControllerImpl the pending_entry_index_ was updated without the pending_entry_. This breaks invariants in the code. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_browser_side_navigation ==========
Description was changed from ========== NavigationController: Fix pending_entry_ pointer not updated. In the NavigationControllerImpl the pending_entry_index_ was updated without the pending_entry_. This breaks invariants in the code. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_browser_side_navigation ========== to ========== NavigationController: Fix pending_entry_ pointer not updated. In the NavigationControllerImpl the pending_entry_index_ was updated without the pending_entry_. This breaks invariants in the code. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by arthursonzogni@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 checked by arthursonzogni@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 checked by arthursonzogni@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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
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 checked by arthursonzogni@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...
Description was changed from ========== NavigationController: Fix pending_entry_ pointer not updated. In the NavigationControllerImpl the pending_entry_index_ was updated without the pending_entry_. This breaks invariants in the code. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== This CL makes that the following invariant stay true: If (pending_entry_index != -1) Then (pending_entry_ = entries[pending_entry_index_]) There was several parts of the code make it becomes false. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
Description was changed from ========== This CL makes that the following invariant stay true: If (pending_entry_index != -1) Then (pending_entry_ = entries[pending_entry_index_]) There was several parts of the code make it becomes false. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== This CL makes that the following invariant stay true: If (pending_entry_index != -1) Then (pending_entry_ = entries[pending_entry_index_]) There were several parts of the code that make it become false. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
The CQ bit was checked by arthursonzogni@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by arthursonzogni@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_...)
The CQ bit was checked by arthursonzogni@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_...)
The CQ bit was checked by arthursonzogni@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...
On 2017/04/13 15:24:30, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... FYI: I found that the assertion: DCHECK_EQ(last_committed_entry_index_ == -1, GetEntryCount() == 0); is not true when a transient entry is added before that any entry are committed. There is a test for this: WebContentsImplTest.ShowInterstitialThenNavigate
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
arthursonzogni@chromium.org changed reviewers: + creis@chromium.org
Hi Charlie, Here is the CL that fixes the problem you found in: https://codereview.chromium.org/2815753003/ There is several places where the class invariant wasn't preserved. Please take a look. Could you also tell me which DCHECK you would like to remove if any. Thanks!
Thanks! I guess the invariant used to be that session history navigations started by just setting the index, at least until getting to NavigateToPendingEntry. The new approach does seem easier to reason about. Mostly looks good, though I'm still sanity checking everything (and looking for an alternative to checking PageTransition, which I tend to avoid relying on). One question before I head out, though-- why did {Set,Discard}TransientEntry need to change?
On 2017/04/14 23:25:31, Charlie Reis wrote: > Thanks! I guess the invariant used to be that session history navigations > started by just setting the index, at least until getting to > NavigateToPendingEntry. The new approach does seem easier to reason about. > > Mostly looks good, though I'm still sanity checking everything (and looking for > an alternative to checking PageTransition, which I tend to avoid relying on). > One question before I head out, though-- why did {Set,Discard}TransientEntry > need to change? What do you mean? This need to change because when entries_ are modified, for instance when a transient entry is deleted or inserted, then the pending_entry_index_ should be updated (the index of the pending entry might decrease or increase). I have made an example without this two pair of lines here: https://codereview.chromium.org/2828443003/ The test SafeBrowsingBlockingPageTest.NavigatingBackAndForth triggers some DCHECK.
Thanks! Looks good with a few suggestions, but one remaining question about the transient case. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:282: DCHECK_EQ(pending_entry_index_, -1); nit: Swap order (expected, actual). Same in all the cases below. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:545: DCHECK_LT(last_committed_entry_index_, GetEntryCount()); Can we also include the second DCHECK from my CL (below), or did that not work? DCHECK_EQ(last_committed_entry_index_ == -1, GetEntryCount() == 0); That's what the second sentence is about, and it doesn't strictly follow from the DCHECK_LT. Currently, it could be -1 when there are entries. If that can happen in some cases (e.g., duplicating a tab?), then we should rephrase or remove the second sentence in the comment. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1861: ui::PAGE_TRANSITION_FORWARD_BACK) && Let's avoid using PageTransition for any meaningful navigation decisions, since I don't trust it in all cases. Can we check for pending_entry_ && pending_entry_index_ == -1 instead? I think that equates to a non-session-history pending navigation now. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:2131: if (pending_entry_index_ > transient_entry_index_) (Same question as below.) https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:2160: if (pending_entry_index_ >= index) Thanks for pointing me to the CL showing the failure. However, there's still something I don't yet understand. The rest of this CL seems like it isn't really changing behavior, other than an internal implementation detail that pending_entry_index_ and pending_entry_ always agree now. But this looks like a definite behavior change, and one that affects test outcomes. Does this mean we had a bug before, and your new DCHECKs caught it? That is, was the GetPendingEntry() call from FocusLocationBarByDefault returning the wrong entry before? I just want to understand this a bit more before we land it, to make sure we aren't regressing anything with the behavior change.
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #9 (id:240001) has been deleted
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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by arthursonzogni@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...
Patchset #9 (id:260001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #4 (id:180001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by arthursonzogni@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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Thanks! Some answers below. The site_isolation bot is red for a bunch of WebKit tests. I checked it was not due to this CL. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:282: DCHECK_EQ(pending_entry_index_, -1); On 2017/04/24 03:58:13, Charlie Reis wrote: > nit: Swap order (expected, actual). > > Same in all the cases below. Done. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:545: DCHECK_LT(last_committed_entry_index_, GetEntryCount()); On 2017/04/24 03:58:13, Charlie Reis wrote: > Can we also include the second DCHECK from my CL (below), or did that not work? > DCHECK_EQ(last_committed_entry_index_ == -1, GetEntryCount() == 0); > > That's what the second sentence is about, and it doesn't strictly follow from > the DCHECK_LT. Currently, it could be -1 when there are entries. If that can > happen in some cases (e.g., duplicating a tab?), then we should rephrase or > remove the second sentence in the comment. We can't because: https://codereview.chromium.org/2810173002/#msg40 ``` FYI: I found that the assertion: DCHECK_EQ(last_committed_entry_index_ == -1, GetEntryCount() == 0); is not true when a transient entry is added before that any entry are committed. There is a test for this: WebContentsImplTest.ShowInterstitialThenNavigate ``` Then I don't know if it is possible to add transient entry without having committed any entry outside of tests. I will update the comment in my next patch. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1861: ui::PAGE_TRANSITION_FORWARD_BACK) && On 2017/04/24 03:58:13, Charlie Reis wrote: > Let's avoid using PageTransition for any meaningful navigation decisions, since > I don't trust it in all cases. > > Can we check for pending_entry_ && pending_entry_index_ == -1 instead? I think > that equates to a non-session-history pending navigation now. Okay, please note that pending_entry_ is already non-null since this CL. I have also added a DCHECK(pending_entry_) above. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:2160: if (pending_entry_index_ >= index) On 2017/04/24 03:58:13, Charlie Reis wrote: > Thanks for pointing me to the CL showing the failure. However, there's still > something I don't yet understand. The rest of this CL seems like it isn't > really changing behavior, other than an internal implementation detail that > pending_entry_index_ and pending_entry_ always agree now. But this looks like a > definite behavior change, and one that affects test outcomes. > > Does this mean we had a bug before, and your new DCHECKs caught it? That is, > was the GetPendingEntry() call from FocusLocationBarByDefault returning the > wrong entry before? > > I just want to understand this a bit more before we land it, to make sure we > aren't regressing anything with the behavior change. Maybe we didn't catch it before, because pending_entry_index_ was wrong only for a short time between the call to SetTransientEntry and and DiscardTransientEntry. Two bugs that compensate each other :-) There was no problem with FocusLocationBarByDefault despite the bug since it only use GetPendingEntry(), and GetPendingEntry() doesn't use pending_entry_index_ at all. Outside of the NavigationController, GetPendingEntryIndex() is mainly used to check if it isn't equals to -1. Which is not a problem. There is 2 functions that are using GetPendingEntryIndex() for something else: - SessionService::BuildCommandsForTabs - TabContentsSyncedTabDelegate::GetPossiblyPendingEntryAtIndex TabContentsSyncedTabDelegate::GetPossiblyPendingEntryAtIndex is weird because it could be replaced by GetEntryAtIndex if we assume that the class invariant is true. I don't really know how the transient entry are used.
Thanks! That helps me understand it. One suggestion for a new test and I think we should be set. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:1861: ui::PAGE_TRANSITION_FORWARD_BACK) && On 2017/04/24 16:28:17, arthursonzogni wrote: > On 2017/04/24 03:58:13, Charlie Reis wrote: > > Let's avoid using PageTransition for any meaningful navigation decisions, > since > > I don't trust it in all cases. > > > > Can we check for pending_entry_ && pending_entry_index_ == -1 instead? I > think > > that equates to a non-session-history pending navigation now. > > Okay, please note that pending_entry_ is already non-null since this CL. I have > also added a DCHECK(pending_entry_) above. Acknowledged. https://codereview.chromium.org/2810173002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:2160: if (pending_entry_index_ >= index) On 2017/04/24 16:28:17, arthursonzogni wrote: > On 2017/04/24 03:58:13, Charlie Reis wrote: > > Thanks for pointing me to the CL showing the failure. However, there's still > > something I don't yet understand. The rest of this CL seems like it isn't > > really changing behavior, other than an internal implementation detail that > > pending_entry_index_ and pending_entry_ always agree now. But this looks like > a > > definite behavior change, and one that affects test outcomes. > > > > Does this mean we had a bug before, and your new DCHECKs caught it? That is, > > was the GetPendingEntry() call from FocusLocationBarByDefault returning the > > wrong entry before? > > > > I just want to understand this a bit more before we land it, to make sure we > > aren't regressing anything with the behavior change. > > Maybe we didn't catch it before, because pending_entry_index_ was wrong only for > a short time between the call to SetTransientEntry and and > DiscardTransientEntry. Two bugs that compensate each other :-) More specifically, I think this means we might have had a bug any time GetPendingEntryIndex() is called while an interstitial is showing (and when it's larger than the interstitial's transient index)? Seems like it might have been off-by-one that whole time, until the interstitial is dismissed. > There was no problem with FocusLocationBarByDefault despite the bug since it > only use GetPendingEntry(), and GetPendingEntry() doesn't use > pending_entry_index_ at all. Yes, I think I agree-- pending_entry_ doesn't seem like it would be affected. > > Outside of the NavigationController, GetPendingEntryIndex() is mainly used to > check if it isn't equals to -1. Which is not a problem. > > There is 2 functions that are using GetPendingEntryIndex() for something else: > - SessionService::BuildCommandsForTabs > - TabContentsSyncedTabDelegate::GetPossiblyPendingEntryAtIndex > > TabContentsSyncedTabDelegate::GetPossiblyPendingEntryAtIndex is weird because it > could be replaced by GetEntryAtIndex if we assume that the class invariant is > true. Yes, maybe this off-by-one bug is the reason people went through such strange contortions to get the pending entry in places like GetPossiblyPendingEntryAtIndex. I agree we should be able to simplify it now, thanks to this fix. To be sure, can you add a unit test to navigation_controller_impl_unittest.cc showing that GetPendingEntryIndex() was wrong when adding a transient entry on a forward navigation, and that your CL fixes it? I would expect that test to fail even without all the DCHECKs, and that will improve our test coverage in this area. (That said, I can't see it explaining the code or the crash in WebContentsState::GetContentsStateAsByteBuffer.) > I don't really know how the transient entry are used. They're used for interstitial pages, and represent entries that will go away as soon as you leave the interstitial. They're certainly tricky to reason about, though. https://codereview.chromium.org/2810173002/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2810173002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:544: // entries. If there is no entries, it must be -1. On the other side, when nit: s/is/are/ https://codereview.chromium.org/2810173002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:546: // entry can be inserted before any navigation. Thanks for clarifying. Let's shorten these last two sentences to: However, there may be a transient entry while the last committed entry index is still -1. https://codereview.chromium.org/2810173002/diff/300001/content/public/browser... File content/public/browser/navigation_controller.h (right): https://codereview.chromium.org/2810173002/diff/300001/content/public/browser... content/public/browser/navigation_controller.h:272: // no entries. As you pointed out, this isn't true. Let's rephrase: It will be -1 if there are no entries, or if there is a transient entry before the first entry commits.
The CQ bit was checked by arthursonzogni@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...
Patchset #8 (id:320001) has been deleted
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Patchset #6 (id:280001) has been deleted
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: This issue passed the CQ dry run.
Thanks! I addressed your comments and added a new test.
The CQ bit was checked by arthursonzogni@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: This issue passed the CQ dry run.
Patchset #7 (id:340001) has been deleted
Fantastic-- new test looks good and fails exactly where I guessed it would. LGTM!
I'll go ahead and CQ, per Nasko's advice.
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 360001, "attempt_start_ts": 1493163305828740, "parent_rev": "fa4db17d5939f767c11d1cae3fe2139dcf928400", "commit_rev": "5c4c202d728cacf71c4ac518e9ab5ee8440eafcf"}
Message was sent while issue was closed.
Description was changed from ========== This CL makes that the following invariant stay true: If (pending_entry_index != -1) Then (pending_entry_ = entries[pending_entry_index_]) There were several parts of the code that make it become false. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== This CL makes that the following invariant stay true: If (pending_entry_index != -1) Then (pending_entry_ = entries[pending_entry_index_]) There were several parts of the code that make it become false. The bug was discovered thanks to [Add DCHECKs for invariants on NavigationController indices.] (https://codereview.chromium.org/2815753003/) BUG=709431 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2810173002 Cr-Commit-Position: refs/heads/master@{#467164} Committed: https://chromium.googlesource.com/chromium/src/+/5c4c202d728cacf71c4ac518e9ab... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:360001) as https://chromium.googlesource.com/chromium/src/+/5c4c202d728cacf71c4ac518e9ab...
Message was sent while issue was closed.
Thanks for the review and for having checked the CQ button. I am OOO until Tuesday. https://codereview.chromium.org/2810173002/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2810173002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:544: // entries. If there is no entries, it must be -1. On the other side, when On 2017/04/24 17:34:55, Charlie Reis wrote: > nit: s/is/are/ Done. https://codereview.chromium.org/2810173002/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:546: // entry can be inserted before any navigation. On 2017/04/24 17:34:55, Charlie Reis wrote: > Thanks for clarifying. Let's shorten these last two sentences to: > > However, there may be a transient entry while the last committed entry index is > still -1. Done. https://codereview.chromium.org/2810173002/diff/300001/content/public/browser... File content/public/browser/navigation_controller.h (right): https://codereview.chromium.org/2810173002/diff/300001/content/public/browser... content/public/browser/navigation_controller.h:272: // no entries. On 2017/04/24 17:34:55, Charlie Reis wrote: > As you pointed out, this isn't true. Let's rephrase: > > It will be -1 if there are no entries, or if there is a transient entry before > the first entry commits. Done. |