@creis: I'm not sure why you're saying once 768526 is fixed? 768526 existed before plznavigate, so I think these are orthogonal issues.

@jam: I don't see how Cast can work with PlzNavigate enabled until issue 768526 is fixed.  It will fail a CHECK in a very common case for them, basically making it non-functional.  Additionally, the CHECK is for security purposes, so I don't see us turning it off.

Is there another way to enable PlzNavigate there and still have Cast be functional, without fixing issue 768526?

@creis: at a high level, before/after plznavigate the security behavior is the same. So for some reason, the CHECK is only firing with PlzNavigate. We should understand why that's the case (i.e. is it URL rewriting and non-plznavigate case is checking pre-rewritten?), and then perhaps we can disable the check in chromecast only for that case/URL.

and to be explicit, if it's not clear: I would like us to avoid delaying removing old navigation code path by a milestone because of the possible-security issue which has existed for a while.

I agree that the first item here is determining why the CHECK fails in PlzNavigate and not without it.  That will help us understand what the next step is.

For reference, the CHECK only fails in PlzNavigate because PlzNavigate is more comprehensive about calling CheckWebUIRendererDoesNotDisplayNormalURL for subframes.  When PlzNavigate is disabled, that function is called from NavigatorImpl::NavigateToEntry, which doesn't get called for renderer-initiated subframe navigations.  (Interestingly, it does get called for subframe back/forward navigations, so going back or forward in the subframe would have crashed.)

At any rate, PlzNavigate is an improvement here by making sure we always call it for any navigation.

I agree that issue 768526 is equally bad in PlzNavigate and non-PlzNavigate, so we might as well land the workaround CLs to turn PlzNavigate back on while the real fix is being worked on.  slan@ has those CLs in progress.

We have identified and fixed the bug causing failed navigations with PlzNavigate on Cast. We were using a NetworkDelegate to append a query parameter on all URL requests (this is due to some legacy features on Cast). This parameter was also being appended to blob requests, causing the navigation stack to fail to commit the navigation.

Thanks to Nasko for helping me dig in.

I will be removing this code if possible after speaking with the right folks. If it cannot be removed, I will add a test upstream to catch this behavior before it lands in Chromium. Thanks all for the patience!

Internal CL here: http://eureka-internal/109865
Public CL here: https://crrev.com/c/721581

Both will land today.

[Updating component - please fix if this isn't correct.]

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

commit dfb45dc31ccd85b9eea9ca5fe194fa4afb77a49c
Author: Stephen Lanham <slan@google.com>
Date: Fri Oct 27 00:11:54 2017

Add BrowserContentClient::IsURLAcceptableForWebUI().

This method is queried by the WebUIControllerFactoryRegistry to allow an
embedder to whitelist a URL for display in a WebUI. This is needed to
work around a long-standing bug in Cast that was discovered when
PlzNavigate was enabled. This is a temporary solution until the bug is
fixed in the correct manner.

Re-enable PlzNavigate on Cast, which now works properly.

Merge-With: eureka-internal/109865

Test: Cast works with PlzNavigate enabled.
BUG= 775089 
BUG=b/67864604

Change-Id: I593206805e74147e535044a1ac2662721cd5efd7
Reviewed-on: https://chromium-review.googlesource.com/721581
Commit-Queue: Stephen Lanham <slan@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512032}
[modify] https://crrev.com/dfb45dc31ccd85b9eea9ca5fe194fa4afb77a49c/chromecast/browser/cast_browser_main_parts.cc
[modify] https://crrev.com/dfb45dc31ccd85b9eea9ca5fe194fa4afb77a49c/chromecast/renderer/cast_content_renderer_client.cc
[modify] https://crrev.com/dfb45dc31ccd85b9eea9ca5fe194fa4afb77a49c/content/browser/webui/web_ui_controller_factory_registry.cc
[modify] https://crrev.com/dfb45dc31ccd85b9eea9ca5fe194fa4afb77a49c/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/dfb45dc31ccd85b9eea9ca5fe194fa4afb77a49c/content/public/browser/content_browser_client.h

Awesome, thank you!

This has landed on our internal branches, so it is closed from Cast's perspective (this does not need to be cherry-picked to M63).

clamy@, I will bounce to you to confirm and close. 

Is there anything actionable we can do on the chromium side for confirm/close?

Also, just to be explicit: we are not planning to merge this CL for M63, unless otherwise indicated by slan@/chromecast team. In addition, we will remove the old navigation code in M65, so there is no turning back :).

Nothing actionable :) Close it out!

No merge into M63, then cleaning up in M65, SGTM. Onward and upward!  

Thanks for confirming! Closing.