Calling browserWindow.loadURL(url) a second time resolves (it should reject again)

[ibc]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a bug report that matches the one I want to file, without success.

Electron Version

28.0.0

What operating system are you using?

macOS

Operating System Version

Ventura 13.6.1

What arch are you using?

x64

Last Known Working Electron version

No response

Expected Behavior

Run this code in Electron having Internet disconnected:

const win = new BrowserWindow();

console.warn('--- calling win.loadURL()');

try {
  await win.loadURL('https://foooooo.lalalalala.bar');

  console.warn('--- win.loadURL() succeeded the first time');
} catch (error) {
  console.warn('--- win.loadURL() failed, trying again');

  try {
    await win.loadURL('https://foooooo.lalalalala.bar');

    console.warn('--- win.loadURL() succeeded the second time');
  } catch (error) {
    console.warn('--- win.loadURL() failed again');
  }
}

console.warn('--- done');

The expected behavior is that win.loadURL() fails (rejects) twice, so I should see these logs:

--- calling win.loadURL()
--- win.loadURL() failed, trying again
--- win.loadURL() failed again
--- done

Actual Behavior

However this is the output I get:

--- calling win.loadURL()
(node:33087) electron: Failed to load URL: https://foooooo.lalalalala.bar/ with error: ERR_INTERNET_DISCONNECTED
(Use `Electron --trace-warnings ...` to show where the warning was created)
--- win.loadURL() failed, trying again
--- win.loadURL() succeeded the second time
--- done

This is, calling win.loadURL() a second time, with same arguments and Internet also disconnected, now resolves instead of rejecting with same error as before. Idempotency is missing here. If calling a method with N arguments under X conditions produces an error result, calling same method again, with same arguments and under same X conditions should produce the same error result. However, here the second time it's producing a positive result.

The interesting thing is that this warning is logged both times: Failed to load URL: ...:

so it looks like, once loadURL() failed for first time, some "error" state was kept and second time loadURL() is called and fails again (for the very same reason than the first time) the code says "oh, it already failed before so let's resolve this time instead of rejecting with same error as before". This is of course a bug.

Testcase Gist URL

No response

Additional Information

No response

[tzahola]

The issue seems to be that on load error, first a did-fail-load is emitted, then a did-finish-load. However, the code in web-contents.ts rejects the loadURL() promise as soon as it receives did-fail-load. Now, if the client code catches the error from the promise, and immediately issues another loadURL() call, that will pick up did-finish-load from the previous invocation, and thus wrongly finish with success.

I've verified that the issue goes away with this change in web-contents.ts:

diff --git a/lib/browser/api/web-contents.ts b/lib/browser/api/web-contents.ts
index 0372d31cea..25d168afc9 100644
--- a/lib/browser/api/web-contents.ts
+++ b/lib/browser/api/web-contents.ts
@@ -429,12 +429,23 @@ WebContents.prototype.loadURL = function (url, options) {
       removeListeners();
       reject(err);
     };
+    let fail = false;
+    let _errorCode = 0;
+    let _errorDescription = "";
+    let _validatedURL = "";
     const finishListener = () => {
-      resolveAndCleanup();
+      if (fail) {
+        rejectAndCleanup(_errorCode, _errorDescription, _validatedURL);
+      } else {
+        resolveAndCleanup();
+      }
     };
     const failListener = (event: Electron.Event, errorCode: number, errorDescription: string, validatedURL: string, isMainFrame: boolean) => {
       if (isMainFrame) {
-        rejectAndCleanup(errorCode, errorDescription, validatedURL);
+        fail = true;
+        _errorCode = errorCode;
+        _errorDescription = errorDescription;
+        _validatedURL = validatedURL;
       }
     };

But this fix assumes that a did-finish-load does always get emitted after a did-fail-load. Can someone chime in and confirm whether this assumption holds true or not?

[georgexu99]

Hey @tzahola! Do you mind opening a quick PR for this issue so we can get more eyes on it to review? 🙇

[tzahola]

Hey @tzahola! Do you mind opening a quick PR for this issue so we can get more eyes on it to review? 🙇

Sure. Here you go: #40661

[electron-issue-triage]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. If you have any new additional information—in particular, if this is still reproducible in the latest version of Electron or in the beta—please include it with your comment!

[electron-issue-triage]

This issue has been closed due to inactivity, and will not be monitored. If this is a bug and you can reproduce this issue on a supported version of Electron please open a new issue and include instructions for reproducing the issue.