Add DCHECKs for invariants on NavigationController indices.

Add DCHECKs for invariants on NavigationController indices.

BUG=
TEST=No DCHECK failures.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[Charlie Reis, 2017-04-11 22:35:03]

Description was changed from

==========
Add DCHECKs for invariants on NavigationController indices.

BUG=
TEST=No DCHECK failures.
==========

to

==========
Add DCHECKs for invariants on NavigationController indices.

BUG=
TEST=No DCHECK failures.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-04-11 23:06:53]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 23:07:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 23:49:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 23:49:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2017-04-11 23:56:40]

creis@chromium.org changed reviewers:
+ arthursonzogni@chromium.org

[Charlie Reis, 2017-04-11 23:56:41]

Arthur, can you patch this in and take a look at the PlzNavigate-specific
failures?  I'm curious if these are real bugs that might explain the crashes in
https://crbug.com/709431 (per https://codereview.chromium.org/2802213002/).

It's worth noting that there's some non-PlzNavigate failures as well, so I'll
try to investigate them on Friday.

[arthursonzogni, 2017-04-12 13:06:55]

On 2017/04/11 23:56:41, Charlie Reis (busy until Fri) wrote:
> Arthur, can you patch this in and take a look at the PlzNavigate-specific
> failures?  I'm curious if these are real bugs that might explain the crashes
in
> https://crbug.com/709431 (per https://codereview.chromium.org/2802213002/).
> 
> It's worth noting that there's some non-PlzNavigate failures as well, so I'll
> try to investigate them on Friday.

Nice!

I have found 2-3 methods in the NavigationControllerImpl that breaks the
invariants.
I have started something here:
https://codereview.chromium.org/2810173002/

But fixing theses method breaks a lot of tests.

I have also found some comments that specify things in conflict with the
invariants:
For instance:
```
  // For session history navigations only the pending_entry_index_ is set.
  if (!pending_entry_) {
    CHECK_NE(pending_entry_index_, -1);
    pending_entry_ = entries_[pending_entry_index_].get();
  }
```

[Charlie Reis, 2017-04-12 16:17:08]

On 2017/04/12 13:06:55, arthursonzogni wrote:
> On 2017/04/11 23:56:41, Charlie Reis (busy until Fri) wrote:
> > Arthur, can you patch this in and take a look at the PlzNavigate-specific
> > failures?  I'm curious if these are real bugs that might explain the crashes
> in
> > https://crbug.com/709431 (per https://codereview.chromium.org/2802213002/).
> > 
> > It's worth noting that there's some non-PlzNavigate failures as well, so
I'll
> > try to investigate them on Friday.
> 
> Nice!
> 
> I have found 2-3 methods in the NavigationControllerImpl that breaks the
> invariants.
> I have started something here:
> https://codereview.chromium.org/2810173002/
> 
> But fixing theses method breaks a lot of tests.
> 
> I have also found some comments that specify things in conflict with the
> invariants:
> For instance:
> ```
>   // For session history navigations only the pending_entry_index_ is set.
>   if (!pending_entry_) {
>     CHECK_NE(pending_entry_index_, -1);
>     pending_entry_ = entries_[pending_entry_index_].get();
>   }
> ```

Thanks!  I vaguely recall running into those before.  I'm busy until Friday, but
can you take a closer look to see if there's a reason for that behavior?  If so,
we can better document and adjust the invariants.  If it works either way, let's
move it to a model that is easier to understand (and keep the proposed
invariants).

[arthursonzogni, 2017-05-10 08:07:33]

On 2017/04/12 16:17:08, Charlie Reis wrote:
> On 2017/04/12 13:06:55, arthursonzogni wrote:
> > On 2017/04/11 23:56:41, Charlie Reis (busy until Fri) wrote:
> > > Arthur, can you patch this in and take a look at the PlzNavigate-specific
> > > failures?  I'm curious if these are real bugs that might explain the
crashes
> > in
> > > https://crbug.com/709431 (per
https://codereview.chromium.org/2802213002/).
> > > 
> > > It's worth noting that there's some non-PlzNavigate failures as well, so
> I'll
> > > try to investigate them on Friday.
> > 
> > Nice!
> > 
> > I have found 2-3 methods in the NavigationControllerImpl that breaks the
> > invariants.
> > I have started something here:
> > https://codereview.chromium.org/2810173002/
> > 
> > But fixing theses method breaks a lot of tests.
> > 
> > I have also found some comments that specify things in conflict with the
> > invariants:
> > For instance:
> > ```
> >   // For session history navigations only the pending_entry_index_ is set.
> >   if (!pending_entry_) {
> >     CHECK_NE(pending_entry_index_, -1);
> >     pending_entry_ = entries_[pending_entry_index_].get();
> >   }
> > ```
> 
> Thanks!  I vaguely recall running into those before.  I'm busy until Friday,
but
> can you take a closer look to see if there's a reason for that behavior?  If
so,
> we can better document and adjust the invariants.  If it works either way,
let's
> move it to a model that is easier to understand (and keep the proposed
> invariants).

I am closing this bug.
The DCHECK have been added and the bugs fixed in
https://codereview.chromium.org/2810173002/

[arthursonzogni, 2017-05-10 08:07:43]

Description was changed from

==========
Add DCHECKs for invariants on NavigationController indices.

BUG=
TEST=No DCHECK failures.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add DCHECKs for invariants on NavigationController indices.

BUG=
TEST=No DCHECK failures.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-05-10 16:48:38]

On 2017/05/10 08:07:33, arthursonzogni wrote:
> I am closing this bug.
> The DCHECK have been added and the bugs fixed in
> https://codereview.chromium.org/2810173002/

Great, thanks!