Repository: electron/electron
Pull request: fix: re-entrancy issues in `webContents.loadURL()`

#### Description of Change

Closes https://github.com/electron/electron/issues/40809

This fixes a crash possible when calling `webContents.loadURL()` from a failed call's catch handler. Essentially, a re-entrant navigation triggered from the `did-start-navigation `event causes the in-flight `NavigationRequest` to be destroyed before Chromium marks it safe, tripping the `is_safe_to_delete_` DCHECK [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=2203?q=navigation_request.cc:2040&sq=package:chromium).

Fix this by monitoring potential re-entrancy sites and failing if we try to call `loadURL` again at a dangerous time.

#### Checklist
<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->

- [x] PR description included and stakeholders cc'd
- [x] `npm test` passes
- [x] [PR release notes](https://github.com/electron/clerk/blob/main/README.md) describe the change in a way relevant to app developers, and are [capitalized, punctuated, and past tense](https://github.com/electron/clerk/blob/main/README.md#examples).

#### Release Notes

Notes: Fixed a crash possible when calling `webContents.loadURL()` from a failed `webContents.loadURL()` call's catch handler.

Changed files:
filename: filenames.auto.gni
patch: @@ -144,6 +144,7 @@ auto_filenames = {
     "docs/api/structures/service-worker-info.md",
     "docs/api/structures/shared-dictionary-info.md",
     "docs/api/structures/shared-dictionary-usage-info.md",
+    "docs/api/structures/shared-texture-handle.md",
     "docs/api/structures/shared-worker-info.md",
     "docs/api/structures/sharing-item.md",
     "docs/api/structures/shortcut-details.md",

filename: shell/browser/api/electron_api_web_contents.cc
patch: @@ -2023,16 +2023,21 @@ SkRegion* WebContents::draggable_region() {
 
 void WebContents::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
+  base::AutoReset<bool> resetter(&is_safe_to_delete_, false);
   EmitNavigationEvent("did-start-navigation", navigation_handle);
 }
 
 void WebContents::DidRedirectNavigation(
     content::NavigationHandle* navigation_handle) {
+  base::AutoReset<bool> resetter(&is_safe_to_delete_, false);
   EmitNavigationEvent("did-redirect-navigation", navigation_handle);
 }
 
 void WebContents::ReadyToCommitNavigation(
     content::NavigationHandle* navigation_handle) {
+  base::AutoReset<bool> resetter(&is_safe_to_delete_, false);
+  EmitNavigationEvent("-ready-to-commit-navigation", navigation_handle);
+
   // Don't focus content in an inactive window.
   if (!owner_window())
     return;
@@ -2375,7 +2380,7 @@ void WebContents::LoadURL(const GURL& url,
   // http://crbug.com/347742.
   auto& ctrl_impl = static_cast<content::NavigationControllerImpl&>(
       web_contents()->GetController());
-  if (ctrl_impl.in_navigate_to_pending_entry()) {
+  if (!is_safe_to_delete_ || ctrl_impl.in_navigate_to_pending_entry()) {
     Emit("did-fail-load", static_cast<int>(net::ERR_FAILED),
          net::ErrorToShortString(net::ERR_FAILED), url.possibly_invalid_spec(),
          true);

filename: shell/browser/api/electron_api_web_contents.h
patch: @@ -858,6 +858,9 @@ class WebContents final : public ExclusiveAccessContext,
   const scoped_refptr<base::TaskRunner> print_task_runner_;
 #endif
 
+  // Track navigation state in order to avoid potential re-entrancy crashes.
+  bool is_safe_to_delete_ = true;
+
   // Stores the frame that's currently in fullscreen, nullptr if there is none.
   raw_ptr<content::RenderFrameHost> fullscreen_frame_ = nullptr;
 

filename: spec/api-web-contents-spec.ts
patch: @@ -484,7 +484,7 @@ describe('webContents module', () => {
       }
     });
 
-    it('fails if loadURL is called inside a non-reentrant critical section', (done) => {
+    it('fails if loadURL is called inside did-start-loading', (done) => {
       w.webContents.once('did-fail-load', (_event, _errorCode, _errorDescription, validatedURL) => {
         expect(validatedURL).to.contain('blank.html');
         done();
@@ -497,6 +497,49 @@ describe('webContents module', () => {
       w.loadURL('data:text/html,<h1>HELLO</h1>');
     });
 
+    it('fails if loadurl is called after the navigation is ready to commit', () => {
+      w.webContents.once('did-fail-load', (_event, _errorCode, _errorDescription, validatedURL) => {
+        expect(validatedURL).to.contain('blank.html');
+      });
+
+      // @ts-expect-error internal-only event.
+      w.webContents.once('-ready-to-commit-navigation', () => {
+        w.loadURL(`file://${fixturesPath}/pages/blank.html`);
+      });
+
+      w.loadURL('data:text/html,<h1>HELLO</h1>');
+    });
+
+    it('fails if loadURL is called inside did-redirect-navigation', (done) => {
+      const server = http.createServer((req, res) => {
+        if (req.url === '/302') {
+          res.statusCode = 302;
+          res.setHeader('Location', '/200');
+          res.end();
+        } else if (req.url === '/200') {
+          res.end('ok');
+        } else {
+          res.end();
+        }
+      });
+
+      w.webContents.once('did-fail-load', (_event, _errorCode, _errorDescription, validatedURL) => {
+        expect(validatedURL).to.contain('blank.html');
+        server.close();
+        done();
+      });
+
+      listen(server).then(({ url }) => {
+        w.webContents.once('did-redirect-navigation', () => {
+          w.loadURL(`file://${fixturesPath}/pages/blank.html`);
+        });
+        w.loadURL(`${url}/302`);
+      }).catch(e => {
+        server.close();
+        done(e);
+      });
+    });
+
     it('sets appropriate error information on rejection', async () => {
       let err: any;
       try {