So far:

Creates window.domAutomationController (but I'm not sure where the command is sent [1]):

  RenderFrameImpl::LoadNavigationErrorPageInternal(...) {
    AllowBindings(BINDINGS_POLICY_DOM_AUTOMATION)
    frame_->LoadData(...);
  }

Doesn't seem to work:

- In SSLErrorTabHelper::DidFinishNavigation
- SSLErrorTabHelper::AssociateBlockingPage
- In a SSLErrorTabHelper::RenderFrameCreated implementation of the WebContentsObserver method

The latter depend on https://chromium-review.googlesource.com/c/chromium/src/+/621236

[1] I think that in the past I managed to get this to route to ChromeControllerClient, but not at the moment.

Correction: It works if I do it in SSLErrorTabHelper::AssociateBlockingPage().

(But then the bindings do still exist if I navigate from expired.badssl.com to badssl.com)

> (But then the bindings do still exist if I navigate from expired.badssl.com to badssl.com)

This is a problem that nasko's working on by proposing that we put error pages in their own separate process.

Nasko, what do you think we should do in the meantime? Do you think it's okay to attach dom automation bindings to the normal renderer process behind the feature flag?

> This is a problem that nasko's working on by proposing that we put error pages in their own separate process.

Ah yes, I think you told me that once, but I forgot to add it here. Thanks for putting it down in writing. :-)

In any case, I still have two things to figure out:

1) When should I set BINDINGS_POLICY_DOM_AUTOMATION on the render frame?

My understanding is that this needs to be after the render frame is created (which may already be done long before we hit an error page, or may be done separately for each error once nasko@ implements it), but before the error page navigation commits.

Ideas:

A) A simple solution is to give DOM automation bindings in RenderFrameImpl::LoadNavigationErrorPageInternal, either to all error pages, or only ones with custom HTML. (Or we could attach a bool indicating whether to add bindings?)

B) Do it on the main frame right before SSLErrorNavigationThrottle cancels the deferred navigation.
I presume this would no longer work if error pages get separate processes.

C) Do it during some WebContentsObserver method in SSLErrorNavigationThrottle.
It looks like ReadyToCommitNavigation() is the best for this?

    void SSLErrorTabHelper::ReadyToCommitNavigation(content::NavigationHandle* handle) {
      auto it = blocking_pages_for_navigations_.find(handle->GetNavigationId());
      if (it != blocking_pages_for_navigations_.end()) {
        web_contents()->GetMainFrame()->AllowBindings(content::BINDINGS_POLICY_DOM_AUTOMATION);
      }
    }

If I understand correctly, C) will only allow bindings during committed interstitial error pages, after nasko@ implements the separate process mechanism.

I'm trying out C) for now, but would appreciate any advice.

2) How do I route the commands to ChromeControllerClient/SSLErrorControllerClient?

I think I had this working briefly, but I lost track of it. Does this correspond to the exist routing ID code here?
https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_page_impl.cc?l=603&rcl=d0aa2f4fc305f5cf801ceb43556d37b3ccd7833f

Can we punt on attaching the dom automation bindings for now, i.e. not have this block  issue 752376 ? It seems like we could implement the new behavior for the ChromeControllerClient methods and unit-test it without needing to have the commands hooked up via dom automation bindings quite yet.

Bindings need to be given from the browser process. If the renderer could add bindings itself, this would be a security problem. This means A) is not a possible solution. B) is also unlikely to work, since we need to pick the RenderFrameHost that will render the error page after the cancellation has been performed. C) is probably the best alternative at this point in time, though we should not be doing this until we know how we are isolating interstitials or only do it behind a flag so it doesn't affect shipping code.

estark@ has mentioned before an alternative - use JS bindings similarly to how current error pages work. This is a very nice property, as we don't have to mess with bindings at all and we can tie the permission to access the JS entrypoints in the browser process based on the SiteInstance of the frame (assuming my design doc and planned work pans out).
This does mean more work, but I'm only bringing it up because it was proposed in earlier discussions. The benefit of going that route is that we won't have any more code using DOM Automation bindings outside of tests, which is great to have.

For completeness, the bindings are given to the interstitial specific process in the current path from the InterstitialPageImpl - https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_page_impl.cc?rcl=0fda7c113a86197fd958689b382884f7d59496ca&l=618.

> C) is probably the best alternative at this point in time, though we should not be doing this until we know how we are isolating interstitials or only do it behind a flag so it doesn't affect shipping code.

Yep, we would only do it behind the --committed-interstitials flag and a big fat warning comment.

Just to clarify, my preferred order of operations would be:
1.) Not attach bindings (DOM automation or JS) at all right now. I don't think they're necessary quite yet except to get a subset of tests passing.
2.) Get all other tests passing while the error page process isolation stuff settles down.
3.) Attach JS bindings and get remaining tests passing (those that exercise user interaction with the interstitial).

If we need bindings sooner and can't wait (e.g. having bindings for some reason blocks progress on getting any tests to pass), then I'd say we should do (C) behind the --committed-interstitials flag and switch to JS bindings later or in parallel.

The plan in comment #9 sounds good. Let's attack it that way.

From talking to nasko@ today, I believe we either need DOM automation bindings or JS bindings. (Where JS bindings are preferred, and interstitials currently use DOM automation bindings, possibly for legacy reasons.)

So that means I should aim to do do C) for now, but I still don't know how to route commands to ChromeControllerClient/SSLErrorControllerClient.

The interstitial page receives DOM automation commands as an IPC at [1]. This seems to be done through the render view created at [2][3]. But for C), the render view is already created by the time we set the bindings, right?

jam@, nasko@, creis@: Is there a way for me to hook up the interstitial page's render view for an interstitial during some WebContentsObserver method? (Does that even make sense to say?)

If not, nasko@ pointed me to NetErrorHelper [4], which implements JS bindings for error pages. I think I roughly understand how we would use this approach for interstitials, but:
1) I don't know how to tell when a render frame reached an interstitial error. We can listen for DidFailProvisionalLoad(), but that requires listening for a WebURLError that may correspond to net::ERR_INSECURE_RESPONSE for all interstitials at the moment.
2) I also don't know how to associate the controller for such a helper with the appropriate interstitial page, especially since the helper doesn't have access to a web_contents.

[1] https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_page_impl.cc?l=355&rcl=30a024937a458f12226fd292972c9666d568b087
[2] https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_page_impl.cc?l=621&rcl=99779b65f09bad81a8f7fd35a31fd0a07e0816b4
[3] This code path also appears to setting `is_interstitials_page` at https://cs.chromium.org/chromium/src/chrome/renderer/content_settings_observer.h?l=176&rcl=797edda23a38016d17389f66d1eef4d2658a7b0a , which makes sure that we ignore normal content settings (so that e.g. JS always runs on interstitial pages). Not sure if we need to do that ourselves for committed interstitials.
[4] https://cs.chromium.org/chromium/src/chrome/renderer/net/net_error_helper.h?type=cs

Okay, separating this from  https://crbug.com/752376  as much as possible for now.

Carlos, I think this would be another good one for you. I'll write some context in the design doc and tag you there.

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

commit 2136d4f77a0e768a011f69ebf3a9e2d38d98ce45
Author: Carlos IL <carlosil@chromium.org>
Date: Wed Dec 20 00:18:11 2017

Added mojo interface for interstitial commands.

Added mojo interface to handle interstitial commands and implemented it
in SSLErrorTabHelper, which passes the command to SSLBlockingPage.

Bug:  779782 
Change-Id: I261dddd835a9e5360c214acd6cedd6d67d2d3710
Reviewed-on: https://chromium-review.googlesource.com/833455
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525192}
[modify] https://crrev.com/2136d4f77a0e768a011f69ebf3a9e2d38d98ce45/chrome/browser/ssl/ssl_error_tab_helper.cc
[modify] https://crrev.com/2136d4f77a0e768a011f69ebf3a9e2d38d98ce45/chrome/browser/ssl/ssl_error_tab_helper.h
[modify] https://crrev.com/2136d4f77a0e768a011f69ebf3a9e2d38d98ce45/components/security_interstitials/core/BUILD.gn
[add] https://crrev.com/2136d4f77a0e768a011f69ebf3a9e2d38d98ce45/components/security_interstitials/core/common/interfaces/BUILD.gn
[add] https://crrev.com/2136d4f77a0e768a011f69ebf3a9e2d38d98ce45/components/security_interstitials/core/common/interfaces/OWNERS
[add] https://crrev.com/2136d4f77a0e768a011f69ebf3a9e2d38d98ce45/components/security_interstitials/core/common/interfaces/interstitial_commands.mojom

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

commit 94293f944d4ada4edae80cbb270db0a498b5aea4
Author: Carlos IL <carlosil@chromium.org>
Date: Thu Jan 11 18:57:22 2018

Changed GetErrorHTML to PrepareErrorPage

Renamed NetErrorHelperCore::GetErrorHTML to PrepareErrorPage and did
similar changes to the methods it calls. Also changed them so they only
add the HTML if the provided string was empty. This is necessary for
committed interstitials (go/committed-interstitials) since GetErrorHTML
also populates pending_error_page_info_, so it needs to be called even
when the HTML string is already populated.

Bug:  779782 
Change-Id: I743338b14667b1bdd46cc38effa974f29813de62
Reviewed-on: https://chromium-review.googlesource.com/853202
Reviewed-by: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528690}
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/android_webview/renderer/aw_content_renderer_client.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/android_webview/renderer/aw_content_renderer_client.h
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/chrome/renderer/chrome_content_renderer_client.h
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/chrome/renderer/net/net_error_helper.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/chrome/renderer/net/net_error_helper.h
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/chrome/renderer/net/net_error_helper_core.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/chrome/renderer/net/net_error_helper_core.h
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/chrome/renderer/net/net_error_helper_core_unittest.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/content/public/renderer/content_renderer_client.h
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/content/renderer/browser_render_view_browsertest.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/content/shell/renderer/shell_content_renderer_client.cc
[modify] https://crrev.com/94293f944d4ada4edae80cbb270db0a498b5aea4/content/shell/renderer/shell_content_renderer_client.h

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

commit 9145731f315bbef3eec9f501526eb98d20400c2e
Author: Carlos IL <carlosil@chromium.org>
Date: Sat Jan 13 00:52:38 2018

Added ssl_certificate_error_page_controller

Added an ssl_certificate_error_page_controller that installs JS
bindings for committed interstitials. Modified net_error_helper so it
implements SSLCertificateErrorPageController::Delegate and changed its
EnablePageHelperFunctions method so it now takes a net error as an
argument and if the error was a certificate error, it calls install
on the new page controller.
Having the interstitial call the new bindings will be added on a
separate CL.

Bug:  779782 
Change-Id: Idd1cc48841d2328bd5ba732d6c1f03d576e3b8e2
Reviewed-on: https://chromium-review.googlesource.com/862613
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Julia Tuttle <juliatuttle@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529123}
[modify] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/BUILD.gn
[modify] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/net/DEPS
[modify] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/net/net_error_helper.cc
[modify] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/net/net_error_helper.h
[modify] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/net/net_error_helper_core.cc
[modify] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/net/net_error_helper_core.h
[modify] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/net/net_error_helper_core_unittest.cc
[add] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/ssl/DEPS
[add] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/ssl/OWNERS
[add] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/ssl/ssl_certificate_error_page_controller.cc
[add] https://crrev.com/9145731f315bbef3eec9f501526eb98d20400c2e/chrome/renderer/ssl/ssl_certificate_error_page_controller.h

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

commit 824db4d831a994dcbdccd0ab23524a4b15863877
Author: Carlos IL <carlosil@chromium.org>
Date: Thu Jan 18 20:44:06 2018

Committed interstitials now handle user actions through JS bindings.

Modified interstitial_common.js so that if a
certificateErrorPageController object exists, interstitial commands
will be executed through the Javascript methods in that object instead
of through DOM automation bindings. Added tests that verify methods in
that object work when committed interstitials are enabled.

Bug:  779782 
Change-Id: I02f6125f1435257c7302798404d6f87f3366cd48
Reviewed-on: https://chromium-review.googlesource.com/869190
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530259}
[modify] https://crrev.com/824db4d831a994dcbdccd0ab23524a4b15863877/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/824db4d831a994dcbdccd0ab23524a4b15863877/components/security_interstitials/core/common/resources/interstitial_common.js