Add check for mismatching item and document sequence numbers.

Add check for mismatching item and document sequence numbers.

When a PageState update is received from the renderer process, it must
be for the current session history entry. This means that the sequence
numbers associated with the FrameNavigationEntry should match the ones
coming from the renderer process.
This CL is adding explicit check for this and a DumpWithoutCrashing to
help diagnose what cases can cause a mismatch.

BUG=628677
Committed: https://crrev.com/e3e35c2d36325c10840b60d2a7cb2a7fe1c6cd14
Cr-Commit-Position: refs/heads/master@{#407316}

[nasko, 2016-07-22 22:33:57]

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

[nasko, 2016-07-22 22:33:57]

Hey Charlie,
Can you review this CL for me? It adds a check for the sequence numbers received
with PageState and a DumpWithoutCrashing so we can get an idea what could be the
cause of it.
Thanks in advance!
Nasko

[Charlie Reis, 2016-07-22 22:46:11]

LGTM with nit.  I'm guessing the compile error is unrelated?

https://codereview.chromium.org/2172123004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2172123004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4485: return;
I'm slightly curious if there are ever cases we'd return here, unable to decode
it.  Hopefully not?  (I suppose we can ignore for now.)

https://codereview.chromium.org/2172123004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4491: // TODO(nasko): Generate
a minidump, which can be debugged to understand the
This isn't really a TODO, since we're generating the minidump below.  :)

Maybe separately put a TODO to remove it once we diagnose
https://crbug.com/628677?

[nasko, 2016-07-22 23:02:34]

https://codereview.chromium.org/2172123004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2172123004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4485: return;
On 2016/07/22 22:46:11, Charlie Reis wrote:
> I'm slightly curious if there are ever cases we'd return here, unable to
decode
> it.  Hopefully not?  (I suppose we can ignore for now.)

It would fail only in the case we fail to deserialize the data. If that is the
case, though, I doubt we want to store that object.

https://codereview.chromium.org/2172123004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4491: // TODO(nasko): Generate
a minidump, which can be debugged to understand the
On 2016/07/22 22:46:11, Charlie Reis wrote:
> This isn't really a TODO, since we're generating the minidump below.  :)
> 
> Maybe separately put a TODO to remove it once we diagnose
> https://crbug.com/628677

Hah, this is what I meant to do, but didn't. Fixed!

[nasko, 2016-07-22 23:02:40]

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

[commit-bot: I haz the power, 2016-07-22 23:03:12]

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, 2016-07-23 00:08:59]

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

[commit-bot: I haz the power, 2016-07-23 00:09:00]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-07-23 00:21:07]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-07-23 00:21:08]

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/2172123004/#ps20001
(title: "Fixing the comment.")

[commit-bot: I haz the power, 2016-07-23 00:21:38]

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, 2016-07-23 00:25:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-23 00:28:43]

Description was changed from

==========
Add check for mismatching item and document sequence numbers.

When a PageState update is received from the renderer process, it must
be for the current session history entry. This means that the sequence
numbers associated with the FrameNavigationEntry should match the ones
coming from the renderer process.
This CL is adding explicit check for this and a DumpWithoutCrashing to
help diagnose what cases can cause a mismatch.

BUG=628677
==========

to

==========
Add check for mismatching item and document sequence numbers.

When a PageState update is received from the renderer process, it must
be for the current session history entry. This means that the sequence
numbers associated with the FrameNavigationEntry should match the ones
coming from the renderer process.
This CL is adding explicit check for this and a DumpWithoutCrashing to
help diagnose what cases can cause a mismatch.

BUG=628677

Committed: https://crrev.com/e3e35c2d36325c10840b60d2a7cb2a7fe1c6cd14
Cr-Commit-Position: refs/heads/master@{#407316}
==========

[commit-bot: I haz the power, 2016-07-23 00:28:44]

Patchset 2 (id:??) landed as
https://crrev.com/e3e35c2d36325c10840b60d2a7cb2a7fe1c6cd14
Cr-Commit-Position: refs/heads/master@{#407316}

[nasko, 2016-07-26 19:00:52]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2187633002/ by nasko@chromium.org.

The reason for reverting is: Reverting this change as it has resulted in much
higher number of reports than anticipated. We have enough data to investigate
and can reland a modified version if needed..