New issue
Advanced search Search tips

Issue 618104 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Clean up NavigationControllerImpl::IsURLInPageNavigation

Project Member Reported by creis@chromium.org, Jun 7 2016

Issue description

We currently call NavigationControllerImpl::IsURLInPageNavigation from two places on the same call stack (NavigatorImpl::DidNavigate and NavigationControllerImpl::RendererDidNavigate).

Because this method can kill the renderer process and send a crash dump about the kill, it's problematic to call it twice in a row.  It's also likely wrong in one of the calls, since we change the FrameTreeNode's origin between the two calls.

We should also clean up the checks within IsURLInPageNavigation, since they are partly redundant with each other now (using multiple representations of origin).
 

Comment 1 by pea...@gmail.com, Sep 26 2016

I am reporting this issue.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b0f797b91e108f96e75fc668a12b194b30b640e

commit 1b0f797b91e108f96e75fc668a12b194b30b640e
Author: peary2 <peary2@gmail.com>
Date: Wed Sep 28 17:28:33 2016

Clean up NavigationControllerImpl::IsURLInPageNavigation

IsURLInPageNavigation was incorrectly being called twice.  This led to
duplicate attempts to try to kill the renderer process when it failed,
and it ran in slightly different states each time.  This CL ensures we
only call it once in the correct state.

BUG=618104
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2376643002
Cr-Commit-Position: refs/heads/master@{#421557}

[modify] https://crrev.com/1b0f797b91e108f96e75fc668a12b194b30b640e/AUTHORS
[modify] https://crrev.com/1b0f797b91e108f96e75fc668a12b194b30b640e/content/browser/frame_host/navigation_controller_impl.cc
[modify] https://crrev.com/1b0f797b91e108f96e75fc668a12b194b30b640e/content/browser/frame_host/navigation_controller_impl.h
[modify] https://crrev.com/1b0f797b91e108f96e75fc668a12b194b30b640e/content/browser/frame_host/navigator_impl.cc

Comment 3 by creis@chromium.org, Sep 28 2016

Thanks for the cleanup!  That takes care of the first half, and I'll leave this open until I resolve the second half (fixing the TODOs in IsURLInPageNavigation, which is a bit subtle).

Sign in to add a comment