Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

Issue 2810173002: NavigationController: Fix several broken class invariants. (Closed)

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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+132 lines, -23 lines) Patch
M content/browser/frame_host/navigation_controller_impl.cc View 1 2 3 4 5 6 15 chunks +51 lines, -22 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_unittest.cc View 1 2 3 4 5 6 1 chunk +78 lines, -0 lines 0 comments Download
M content/public/browser/navigation_controller.h View 1 2 3 4 5 6 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 89 (75 generated)
arthursonzogni
On 2017/04/13 15:24:30, commit-bot: I haz the power wrote: > Dry run: CQ is trying ...
3 years, 8 months ago (2017-04-13 15:28:48 UTC) #40
arthursonzogni
Hi Charlie, Here is the CL that fixes the problem you found in: https://codereview.chromium.org/2815753003/ There ...
3 years, 8 months ago (2017-04-14 07:14:48 UTC) #44
arthursonzogni
3 years, 8 months ago (2017-04-14 11:28:46 UTC) #45
Charlie Reis
Thanks! I guess the invariant used to be that session history navigations started by just ...
3 years, 8 months ago (2017-04-14 23:25:31 UTC) #46
arthursonzogni
On 2017/04/14 23:25:31, Charlie Reis wrote: > Thanks! I guess the invariant used to be ...
3 years, 8 months ago (2017-04-18 08:41:53 UTC) #47
Charlie Reis
Thanks! Looks good with a few suggestions, but one remaining question about the transient case. ...
3 years, 8 months ago (2017-04-24 03:58:13 UTC) #48
arthursonzogni
Thanks! Some answers below. The site_isolation bot is red for a bunch of WebKit tests. ...
3 years, 8 months ago (2017-04-24 16:28:17 UTC) #66
Charlie Reis
Thanks! That helps me understand it. One suggestion for a new test and I think ...
3 years, 8 months ago (2017-04-24 17:34:55 UTC) #67
arthursonzogni
Thanks! I addressed your comments and added a new test.
3 years, 8 months ago (2017-04-25 11:22:38 UTC) #76
Charlie Reis
Fantastic-- new test looks good and fails exactly where I guessed it would. LGTM!
3 years, 8 months ago (2017-04-25 23:07:49 UTC) #82
Charlie Reis
I'll go ahead and CQ, per Nasko's advice.
3 years, 8 months ago (2017-04-25 23:34:59 UTC) #83
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2810173002/360001
3 years, 8 months ago (2017-04-25 23:35:39 UTC) #85
commit-bot: I haz the power
Committed patchset #7 (id:360001) as https://chromium.googlesource.com/chromium/src/+/5c4c202d728cacf71c4ac518e9ab5ee8440eafcf
3 years, 8 months ago (2017-04-25 23:41:46 UTC) #88
arthursonzogni
3 years, 8 months ago (2017-04-26 12:13:48 UTC) #89
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.

Powered by Google App Engine
This is Rietveld 408576698