This is also broken by:

*) ChromeRenderFrameObserver::DidCommitProvisionalLoad (executes WebUI javascript right *before* the renderer sends the DidCommitProvisionalLoad IPC)

*) DevToolsClient::DidClearWindowObject

I have tried adding a DCHECK(DidNavigationCommit(frame_)) to RenderFrameImpl::OnJavaScriptExecuteRequest and RenderFrameImpl::OnJavaScriptExecuteRequestInIsolatedWorld (*) where DidNavigationCommit looks as follows:

    bool DidNavigationCommit(WebLocalFrame* frame) {
      DocumentState* document_state =
          DocumentState::FromDocumentLoader(frame->GetDocumentLoader());
      NavigationStateImpl* navigation_state =
          static_cast<NavigationStateImpl*>(document_state->navigation_state());
      return navigation_state->request_committed();
    }

To make the DCHECK pass, I have tried the approaches listed below.


Approach #1 - flip the order of A) RenderFrameObserver::DidCommit... calls and B) DidCommit... IPC.
===================================================================================================

The minimal CL implementing this approach is at https://chromium-review.googlesource.com/c/chromium/src/+/671469/3.

This approach runs into the following issues:

- NTP test failues
    - Somewhat related to r509728
    - Can be fixed by changing SearchBox::DidCommit... to SearchBox::WillCommit...

- PageLoadMetrics test failures:
    - Failing tests:
        - PageLoadMetricsBrowserTest.DocumentWriteReload
        - SubresourceFilterBrowserTest.PageLoadMetrics
        - SessionRestorePageLoadMetricsBrowserTest.NavigationDuringSessionRestore
    - I don't know how to fix these test failures.
        - I cannot repro these failures locally.
        - My hunch is that after the CL, MetricsWebContentsObserver::OnTimingUpdated sees
          the newly committed URL (rather than the old URL that the timing data is for)
          and gets confused - I think in some tests the new URL can be about:blank which
          will cause an early return (because of !SchemeIsHTTPOrHTTPS).


Approach #1b - like approach #1, but also change MetricsRenderFrameObserver::DidCommit to WillCommit...
=======================================================================================================

An example CL with this approach is at https://chromium-review.googlesource.com/#/c/chromium/src/+/745033

This approach runs into the following issues:

- The NTP test failures are fixed (yay!).

- PageLoadMetrics tests continue to fail:

    - Failing tests:
        - MultiTabLoadingPageLoadMetricsBrowserTest.MultiTabForeground
        - MultiTabLoadingPageLoadMetricsBrowserTest.MultiTabBackground
        - PageLoadMetricsBrowserTest.DocumentWriteReload
    - MultiTabLoadingPageLoadMetricsBrowserTest.MultiTabForeground
        - Missing hits of kHistogramMultiTabLoadingLoad
          (PageLoad.Clients.MultiTabLoading.DocumentTiming.NavigationToLoadEventFired)
        - The hits are missed because mojom::DocumentTiming::load_event_start is not set


Approach #2 - postpone individual script executions via PostTask
================================================================

The minimal CL implementing this approach is at https://chromium-review.googlesource.com/c/chromium/src/+/701420

This approach runs into the following issues:

- CvoxBraille* tests fail
    - Example failing test: CvoxBrailleDisplayManagerUnitTest.BasicGroup
    - AFAICT the tests run only on CrOS
    - The tests are built into chromevox_tests executable
    - The tests fail, because chrome/test/data/webui/test_api.js runs before "chrome" (e.g. chrome.send) is bound



(*) Adding the DCHECK to RenderFrameImpl::OnJavaScriptExecuteRequestForTests is also desirable, but is lower priority (and hits its own set of issues).

PS. The DCHECK gets also violated from DevToolsFrontendImpl::DidClearWindowObject.  I believe that this violation can be fixed by PostTask approach.

bmcquade@, could you please comment on the feasibility of changing when exactly MetricsRenderFrameObserver flushes out the data after a new commit (i.e. approaches #1 and #1b in the previous comment)?  Would you have any suggestions on how to proceed?

Thank you for investigating this. I agree it's important to prevent script from executing before commit.

RE: the page load metrics tests, page load metrics, and likely other components elsewhere in Chrome, do depend on the relative ordering of IPCs.

Page load metrics currently requires that the commit IPC and its own mojo message have a certain order. There is some more detail on this here: https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_sII/edit#heading=h.iywi24jw0vm7 (see the 'important but subtle detail').

RE: 1 and 1b, it appears they are both not quite sufficient. 1 will break for reasons explained in the doc above. 1b is getting closer to fixing but I suspect the lack of args in WillCommit and/or subtle differences in when WillCommit vs DidCommit are called is problematic for these tests.

It might be better if Chrome provided an explicit mechanism for people interested in sychronizing with this IPC to run code right before (or after) it is sent, to reduce the fragility. I wonder if there's a mojo mechanism for this.

In the meantime, we probably need to make sure that the relative ordering of these two messages is preserved across any refactors.

And FWIW I suspect page load metrics is just one of a number of bits of code that depends on this ordering - I suspect there is some code without good test coverage that may start failing if we change the ordering.

Thanks for the feedback - because of it, I focused on approach #2 (PostTask).  I think I understand the WebUI-related test failures on CrOS and have a fix for them in https://crrev.com/c/701420.  There is one more CrOS failure that runs into the DCHECK though:

$ DISPLAY=:20 out/cros/browser_tests --gtest_filter=*EnrollmentScreenTest.EnrollmentSpinner*
...
[1:1:1102/080419.489193:FATAL:render_frame_impl.cc(2163)] Check failed: DidNavigationCommit(frame_). login.OAuthEnrollmentScreen.showStep("signin");

FWIW, the browser requests this javascript from the following callstack:

    #1 0x7ff2ca2e36ae content::RenderFrameHostImpl::ExecuteJavaScript()
    #2 0x7ff2ca623d9b content::WebUIImpl::CallJavascriptFunctionUnsafe()
    #3 0x000003b3facc chromeos::EnrollmentScreenHandler::ShowStep()
    #4 <base::Callback binding goo>
    #5 0x000001a2df7a chromeos::EnrollmentScreen::ClearAuth()
    #6 0x000001a2e199 chromeos::EnrollmentScreen::ShowInteractiveScreen()
    #7 0x000001ab9d9f chromeos::WizardController::ShowCurrentScreen()
    #8 0x000001ab98f1 chromeos::WizardController::SetCurrentScreenSmooth()
    #9 0x000001ab71c4 chromeos::WizardController::StartEnrollmentScreen()
    #10 0x000001ab7054 chromeos::WizardController::ShowEnrollmentScreen()
    #11 0x000001ab5c36 chromeos::WizardController::Init()
    #12 0x000001a82dd3 chromeos::LoginDisplayHostImpl::StartWizard()
    #13 0x000001a867c6 (anonymous namespace)::ShowLoginWizardFinish()
    #14 0x000001a85d64 chromeos::ShowLoginWizard()
    #15 0x000001538f92 chromeos::WizardInProcessBrowserTest::SetUpOnMainThread()

I also see that the javascript is executed on an empty, initial document (i.e. when the DCHECK fires I see |render_view_->history_list_length_ == 0| and |GetWebFrame()->GetDocument().Url()| == "").

I'll go ahead and carve out an exception in the DCHECK for initial, empty document.

In this bug we try to prevent *scripts* from executing before a commit, because

1) before a commit IPC, the browser doesn't yet know about the committed origin (for example GetLastCommittedOrigin on the browser side is still stale, the new origin is not yet accounted for in ChildProcessSecurityPolicy)

2) with Site Isolation things like localStorage need to verify the origin requested by a renderer against the browser-side knowledge about the origins committed in the renderer

3) scripts can trigger localStorage access


QUESTION: Are scripts the *only* thing to worry about?  Are there other features that should not, but might possibly execute before the commit IPC?  If there are other features, then is a script-specific DCHECK still valuable (e.g. see https://crrev.com/c/701420/30/content/renderer/render_frame_impl.cc#2185)?