Manual regression range:
Good Build: 52.0.2742.0
Bad Build:  52.0.2743.24 

Narrow bisect:
https://chromium.googlesource.com/chromium/src/+log/e18e3dd1a51cfdf6f278b4986e853a016effd249..6b693cb69c1731ac3e4fa0851e64fd10fa820a1c?pretty=fuller&n=50

Suspecting: r394857

Deleted: Actual_Crash.mov 2.8 MB

Actual_Crash.mov 2.8 MB Download

dbeam/creis:

This bug is a symptom of a nasty root cause.

Back/Forward causes RenderViewReused which causes DisallowJavascript.

However, Settings pushes state onto window.history, so Back and Forward does not reload the page. This means the Polymer elements never call their on-attached chrome.send('init') methods again, and never re-allow JavaScript.

This is bad because of both crashes like above, and also that C++ side observers are now disabled.

I can't modify WebUI::RenderViewReused to ignore BACK_AND_FORWARD transitions, because that would break WebUI pages that don't push state onto window.history.

My initial (painful) recommendation is: Every element needs to observe currentRoute, and redo the chrome.send('init'). That's a lot of work.

Thoughts?

A quicker fix that I can also endorse:

Make SettingsPageUIHandler::RenderViewReused override WebUIMessageHandler::RenderViewReused to ignore BACK_AND_FORWARD transitions.

That way, WebUIs that don't push to window.history still work, and Settings works without modifying a ton of Javascript.

I'm confused about why RenderViewReused is calling DisallowJavascript.  It looks like RenderViewReused is called at the time the navigation *begins*, via RenderFrameHostManager::UpdateStateForNavigate (or GetFrameHostForNavigation in PlzNavigate).  That navigation might never complete-- the user could hit stop before it commits, or it could turn out to be a download or otherwise fail.  Admittedly those would be uncommon on WebUI, but still possible.  In that case, you'd be left on the previous WebUI page and it wouldn't work anymore.

Shouldn't DisallowJavascript be called at commit time instead?  That would let you avoid calling it on in-page navigations, which would probably solve this bug.

creis:

We used RenderViewReused to catch reloads. It does, but maybe it's not quite the right one.

I don't completely understand the navigation cycle. Is there a lifecycle call that triggers on Reload and "real navigate", but not on a navigate that stays on the same page? (i.e. with window.history pushes).

That would be perfect.

Thanks,

Tommy

Yep: use WebContentsObserver::DidFinishNavigation and check NavigationHandle::IsSamePage().  That will return true for in-page navigations like fragments and pushState (which don't leave the current document) and false for cross-document navigations, including reloads.  (I've just confirmed the reload case in a debugger.)

If you're not using a WebContentsObserver, you can do something similar at commit time.  For example, suppose you wanted RenderFrameHostManager::DidNavigateFrame to call out to something like UpdatePendingWebUIOnCurrentFrameHost or RenderViewReused.  You could pass in |is_navigation_within_page| from NavigatorImpl::DidNavigate to check whether it was in-page.  (That may require updating a bunch of unit tests.)

creis:

Thank you! That's very helpful. It looks like I will be doing a bit of refactor work to make the WebUI work correctly with back and forward buttons. I'll mark you as reviewer.

Thanks again!

 Issue 621430  has been merged into this issue.

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

commit e525223a1c71de1106cf8e23c54d239d298d9982
Author: Tommy C. Li <tommycli@chromium.org>
Date: Tue Jun 28 19:07:33 2016

WebUI: DisallowJavascript only on Refresh and non-same-page navigations

Previously, WebUI handlers were being disabled via DisallowJavascript
when the user clicked the browser's Back and Forward buttons, even for
in-page navigations (fragments or window.history.push).

That led to crashes and unexpected behavior.

BUG= 621885 
R=creis@chromium.org, dbeam@chromium.org

Review URL: https://codereview.chromium.org/2099563002 .

Cr-Commit-Position: refs/heads/master@{#402507}

[modify] https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982/chrome/browser/ui/webui/webui_browsertest.cc
[modify] https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982/content/browser/webui/web_ui_impl.cc
[modify] https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982/content/browser/webui/web_ui_impl.h
[modify] https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982/content/browser/webui/web_ui_message_handler.cc
[modify] https://crrev.com/e525223a1c71de1106cf8e23c54d239d298d9982/content/public/browser/web_ui_message_handler.h

TE please verify. Thanks.