fix: don't reject loadURL() promise from did-fail-load - use did-finish-load instead

[tzahola]

Description of Change

Fixes this issue: #40640

The promise from WebContents.loadURL() rejects as soon as it receives did-fail-load. However, did-fail-load will be followed by a did-finish-load, which means that if the client code immediately issues another loadURL() when the promise rejects, this loadURL() will be resolved by the previous call's did-finish-load.

This PR changes the did-fail-load handler to only note the failure, and leave it to did-finish-load to actually resolve/reject the promise.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation, tutorials, templates and examples are changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed WebContents.loadURL() incorrectly failing if called immediately after a previous call to loadURL() failed.

[nornagon]

This seems broadly reasonable, but the WebContentsObserver docs make it seem like did-finish-load won't always be called:

  // This method is invoked when the load is done, i.e. the spinner of the tab
  // will stop spinning, and the onload event was dispatched.
  //
  // If the WebContents is displaying replacement content, e.g. network error
  // pages, DidFinishLoad is invoked for frames that were not sending
  // navigational events before. It is safe to ignore these events.
  virtual void DidFinishLoad(RenderFrameHost* render_frame_host,
                             const GURL& validated_url) {}

  // This method is like DidFinishLoad, but when the load failed or was
  // cancelled, e.g. window.stop() is invoked.
  virtual void DidFailLoad(RenderFrameHost* render_frame_host,
                           const GURL& validated_url,
                           int error_code) {}

How sure are we that did-finish-load is always called after did-fail-load is dispatched?

Also, a relevant test is failing :)

[tzahola]

How sure are we that did-finish-load is always called after did-fail-load is dispatched?

As far as I can tell, the call hierarchy is linear (i.e. no other path leads to WebContentsObserver::DidFailLoad), and looks like this:

1. DocumentLoader::LoadFailed
2. LocalFrameClientImpl::DispatchDidFailLoad
3. WebLocalFrameImpl::DidFailLoad
4. RenderFrameHostImpl::DidFailLoadWithError
5. WebContentsImpl::DidFailLoadWithError
6. WebContentsObserver::DidFailLoad

And DocumentLoader::LoadFailed issues the "failed" and "finished" calls right after another:

void DocumentLoader::LoadFailed(const ResourceError& error) {
...
  state_ = kSentDidFinishLoad;
  GetLocalFrameClient().DispatchDidFailLoad(error, history_commit_type);
  GetFrameLoader().DidFinishNavigation(
      FrameLoader::NavigationFinishState::kFailure);
  DCHECK_EQ(kSentDidFinishLoad, state_);
  params_ = nullptr;
}

[tzahola]

Also, a relevant test is failing 🙂

Yes, it seems that the Electron WebContents is in certain cases dispatching did-fail-load multiple times within a single load sequence, and my implementation returns the last of these.

Actually, it was did-stop-loading emitted after did-fail-load. But now it's fixed. Also added a test (verified that it fails on the current main).

[release-clerk]

Release Notes Persisted

Fixed WebContents.loadURL() incorrectly failing if called immediately after a previous call to loadURL() failed.

[tzahola]

Can we backport this to 27?