Ensure that the browser’s copy of page id is in sync with the renderer’s.

Ensure that the browser’s copy of page id is in sync with the renderer’s.

This might-maybe-hope-not crash, and so has debugging statements.

BUG=407376
TEST=no crashing, we hope
TBR=rsesek@chromium.org

Committed: https://crrev.com/0eba5c5e63e52330d3409193f5184aaffac45e90
Cr-Commit-Position: refs/heads/master@{#308036}

[Avi (use Gerrit), 2014-12-11 20:43:58]

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

[Avi (use Gerrit), 2014-12-11 20:43:58]

Deja vu.

[Charlie Reis, 2014-12-11 22:08:50]

LGTM once https://codereview.chromium.org/790423002/ lands.  Hopefully that's
the last way for this to go wrong!

[Charlie Reis, 2014-12-11 23:35:36]

LGTM once https://codereview.chromium.org/790423002/ lands.  Hopefully
that's the last way for this to go wrong!

On Thu, Dec 11, 2014 at 12:43 PM, <avi@chromium.org> wrote:

> Reviewers: Robert Sesek, Charlie Reis,
>
> Message:
> Deja vu.
>
> Description:
> Ensure that the browser's copy of page id is in sync with the renderer's.
>
> This might-maybe-hope-not crash, and so has debugging statements.
>
> BUG=407376
> TEST=no crashing, we hope
> TBR=rsesek@chromium.org
>
> Please review this at https://codereview.chromium.org/795233002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+35, -1 lines):
>   M chrome/common/crash_keys.cc
>   M content/browser/renderer_host/render_view_host_delegate.h
>   M content/browser/renderer_host/render_view_host_impl.cc
>   M content/browser/web_contents/web_contents_impl.h
>   M content/browser/web_contents/web_contents_impl.cc
>
>
> Index: chrome/common/crash_keys.cc
> diff --git a/chrome/common/crash_keys.cc b/chrome/common/crash_keys.cc
> index b96a3ac36c8c8f0112af854b54c242f0f70cb350..
> 836e1954e072876d7c75f01fccd7dd783dc65297 100644
> --- a/chrome/common/crash_keys.cc
> +++ b/chrome/common/crash_keys.cc
> @@ -167,6 +167,12 @@ size_t RegisterChromeCrashKeys() {
>      { "channel_error_bt", kMediumSize },
>      { "remove_route_bt", kMediumSize },
>      { "rwhvm_window", kMediumSize },
> +    // The following keys are for diagnosing crashes in
> http://crbug.com/369661.
> +    // They will not be permanent.
> +    { "renderer_page_id", kSmallSize },
> +    { "browser_page_id", kSmallSize },
> +    { "last_committed_page_id", kSmallSize },
> +    // End http://crbug.com/369661
>      // media/:
>      { "VideoCaptureDeviceQTKit", kSmallSize },
>  #endif
> Index: content/browser/renderer_host/render_view_host_delegate.h
> diff --git a/content/browser/renderer_host/render_view_host_delegate.h
> b/content/browser/renderer_host/render_view_host_delegate.h
> index 47b5986ce3f53d963d502bdcfd023abe34ee578a..
> 5c0eb8753c37de935ffa0a432d2823f68e323a25 100644
> --- a/content/browser/renderer_host/render_view_host_delegate.h
> +++ b/content/browser/renderer_host/render_view_host_delegate.h
> @@ -106,6 +106,7 @@ class CONTENT_EXPORT RenderViewHostDelegate {
>
>    // The state for the page changed and should be updated.
>    virtual void UpdateState(RenderViewHost* render_view_host,
> +                           int32 rvh_page_id,  // http://crbug.com/407376
>                             int32 page_id,
>                             const PageState& state) {}
>
> Index: content/browser/renderer_host/render_view_host_impl.cc
> diff --git a/content/browser/renderer_host/render_view_host_impl.cc
> b/content/browser/renderer_host/render_view_host_impl.cc
> index b2d8a02e0439f75e48e98ff3d23cb72f3c073f58..
> 05f5421144196885ba85b914df858679df26c7e5 100644
> --- a/content/browser/renderer_host/render_view_host_impl.cc
> +++ b/content/browser/renderer_host/render_view_host_impl.cc
> @@ -1078,7 +1078,7 @@ void RenderViewHostImpl::OnUpdateState(int32
> page_id, const PageState& state) {
>      return;
>    }
>
> -  delegate_->UpdateState(this, page_id, state);
> +  delegate_->UpdateState(this, page_id_, page_id, state);
>  }
>
>  void RenderViewHostImpl::OnUpdateTargetURL(const GURL& url) {
> Index: content/browser/web_contents/web_contents_impl.cc
> diff --git a/content/browser/web_contents/web_contents_impl.cc
> b/content/browser/web_contents/web_contents_impl.cc
> index ca9112af81bc56e40d79e9f6ff6dc120c11d0977..
> 332333c658b392b04ab2fdcf82b93cd3b84d26e1 100644
> --- a/content/browser/web_contents/web_contents_impl.cc
> +++ b/content/browser/web_contents/web_contents_impl.cc
> @@ -7,6 +7,7 @@
>  #include <utility>
>
>  #include "base/command_line.h"
> +#include "base/debug/crash_logging.h"
>  #include "base/debug/trace_event.h"
>  #include "base/lazy_instance.h"
>  #include "base/logging.h"
> @@ -3604,8 +3605,33 @@ void
WebContentsImpl::RenderViewDeleted(RenderViewHost*
> rvh) {
>  }
>
>  void WebContentsImpl::UpdateState(RenderViewHost* rvh,
> +                                  int32 rvh_page_id,
>                                    int32 page_id,
>                                    const PageState& page_state) {
> +  if (rvh_page_id != page_id) {
> +    base::debug::SetCrashKeyValue("renderer_page_id",
> +                                  base::IntToString(page_id));
> +    base::debug::SetCrashKeyValue("browser_page_id",
> +                                  base::IntToString(rvh_page_id));
> +    NavigationEntryImpl* entry = NavigationEntryImpl::
> FromNavigationEntry(
> +        controller_.GetLastCommittedEntry());
> +    // The question being answered here is: when there is a page id
> mismatch
> +    // between the renderer and the browser, which value (if either)
> matches
> +    // that of the last committed entry?
> +    if (entry) {
> +      if (entry->site_instance() == rvh->GetSiteInstance()) {
> +        base::debug::SetCrashKeyValue("last_committed_page_id",
> +                                      base::IntToString(entry->
> GetPageID()));
> +      } else {
> +        base::debug::SetCrashKeyValue("last_committed_page_id",
> +                                      "site instance mismatch");
> +      }
> +    } else {
> +      base::debug::SetCrashKeyValue("last_committed_page_id", "no
> entry");
> +    }
> +    CHECK(false);
> +  }
> +
>    // Ensure that this state update comes from a RenderViewHost that
> belongs to
>    // this WebContents.
>    // TODO(nasko): This should go through RenderFrameHost.
> Index: content/browser/web_contents/web_contents_impl.h
> diff --git a/content/browser/web_contents/web_contents_impl.h
> b/content/browser/web_contents/web_contents_impl.h
> index 339865a636f2d335170927bbd464ecc99f48d1ca..
> 4f134ace12d6f0ee748463716ed804d59824cb20 100644
> --- a/content/browser/web_contents/web_contents_impl.h
> +++ b/content/browser/web_contents/web_contents_impl.h
> @@ -406,6 +406,7 @@ class CONTENT_EXPORT WebContentsImpl
>                              int error_code) override;
>    void RenderViewDeleted(RenderViewHost* render_view_host) override;
>    void UpdateState(RenderViewHost* render_view_host,
> +                   int32 rvh_page_id,
>                     int32 page_id,
>                     const PageState& page_state) override;
>    void UpdateTargetURL(const GURL& url) override;
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2014-12-12 01:47:44]

Surely this will work.

[Avi (use Gerrit), 2014-12-12 01:47:51]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-12-12 01:49:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/795233002/1

[commit-bot: I haz the power, 2014-12-12 02:48:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-12-12 02:49:21]

Patchset 1 (id:??) landed as
https://crrev.com/0eba5c5e63e52330d3409193f5184aaffac45e90
Cr-Commit-Position: refs/heads/master@{#308036}

[Avi (use Gerrit), 2014-12-13 20:32:25]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/803803003/ by avi@chromium.org.

The reason for reverting is: Still crashing..