Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Unsanboxed preload running after page with content is loaded (ESM) #40777

Open
3 tasks done
Arkellys opened this issue Dec 16, 2023 · 18 comments
Open
3 tasks done
Labels
bug 🪲 has-repro-comment Issue has repro in comments

Comments

@Arkellys
Copy link

Preflight Checklist

Electron Version

28.0.0

What operating system are you using?

Windows

Operating System Version

Windows 10 Family

What arch are you using?

x64

Last Known Working Electron version

No response

Expected Behavior

Electron 28 added support for ESM, the new doc states that:

Unsandboxed ESM preload scripts will run after page load on pages with no content

If the response body for a renderer's loaded page is completely empty (i.e. Content-Length: 0), its preload script will not block the page load, which may result in race conditions.

If this impacts you, change your response body to have something in it (e.g. an empty html tag (<html></html>)) or swap back to using a CommonJS preload script (.js or .cjs), which will block the page load.

So for an HTML with content, I expect the preload to be run before the page loads:

image

Actual Behavior

For the following HTML:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <meta http-equiv="Content-Security-Policy" content="script-src 'self';">
  </head>
	
  <body>
    <main>
      <p>
        There is nothing interesting to see here...
      </p>
    </main>
  </body>
</html>

Loaded as:

new BrowserWindow({
  webPreferences: {
    nodeIntegration: false,
    contextIsolation: true,
    sandbox: false,
    preload: /*preload path */,
  }
});

With in package.json:

"type": "module"

The JS file linked to my HTML shows the page is loaded before the preload.

image

For context, listen is a function exposed on the preload with contextBridge.

Testcase Gist URL

No response

Additional Information

I can't see the Content-Length header in the response headers:

image

The file is loaded from http://localhost:3000/worker.html

@Arkellys Arkellys changed the title [Bug]: Unsanboxed preload running after page with content is loaded [Bug]: Unsanboxed preload running after page with content is loaded (ESM) Dec 16, 2023
@Arkellys
Copy link
Author

I can see a Content-Length when my renderer is fast-refreshed with webpack, but the preload is still run after the page.

image

@MarshallOfSound
Copy link
Member

@Arkellys Without a full repro there's not much to look at here, but per our docs does your preload script have the mjs file extension, just setting type: module is not enough

@Arkellys
Copy link
Author

Arkellys commented Dec 17, 2023

@Arkellys Without a full repro there's not much to look at here, but per our docs does your preload script have the mjs file extension, just setting type: module is not enough

I thought the example was simple enough that it didn't need a repo, but I can make one if you think it's necessary? And yes the preload has .mjs extension, I was getting require error without it.

@nbawp1
Copy link

nbawp1 commented Dec 18, 2023

Can confirm that this is also happening to our app, though intermittently, so clearly a race-condition.

@jkleinsc jkleinsc added the blocked/need-repro Needs a test case to reproduce the bug label Jan 8, 2024
@electron-issue-triage
Copy link

Hello @Arkellys. Thanks for reporting this and helping to make Electron better!

Would it be possible for you to make a standalone testcase with only the code necessary to reproduce the issue? For example, Electron Fiddle is a great tool for making small test cases and makes it easy to publish your test case to a gist that Electron maintainers can use.

Stand-alone test cases make fixing issues go more smoothly: it ensure everyone's looking at the same issue, it removes all unnecessary variables from the equation, and it can also provide the basis for automated regression tests.

Now adding the blocked/need-repro Needs a test case to reproduce the bug label for this reason. After you make a test case, please link to it in a followup comment. This issue will be closed in 10 days if the above is not addressed.

@compeek
Copy link

compeek commented Jan 10, 2024

I'm encountering this as well after upgrading to Electron 28 and switching to ESM.

Similar to the OP, my preload script (ending in .mjs as required) uses contextBridge.exposeInMainWorld(), and my Angular app in the renderer process looks for that value that was set. It's usually fine on the first load, but after a refresh, it errors out immediately and says the thing that was supposed to be set is undefined. If I disable the cache in the browser developer tools, then it works again.

It does look to me like the preload script is not blocking the page load like it should, and there's a race condition regarding whether the preload script runs quickly enough before the page loads.

@electron-issue-triage electron-issue-triage bot removed the blocked/need-repro Needs a test case to reproduce the bug label Jan 10, 2024
@nbawp1
Copy link

nbawp1 commented Jan 10, 2024

I am able to reproduce every time with the following setup:

main.js

import { app, BrowserWindow } from "electron";
import path from "node:path";
import { createServer } from "node:http";

const html = String.raw;

(async () => {
  await app.whenReady();

  const server = createServer((req, res) => {
    if (req.url === "/") {
      res.writeHead(200, { "Content-Type": "text/html" });
      res.end(
        html`<html>
          <head>
            <meta charset="utf-8" />
            <title>Electron test</title>
            <script type="module" src="renderer.js"></script>
          </head>
          <body></body>
        </html>`
      );
    } else if (req.url === "/renderer.js") {
      res.writeHead(200, { "Content-Type": "text/javascript" });
      res.end(`console.log("renderer loaded");`);
    }
  });

  server.listen(8080);

  const win = new BrowserWindow({
    webPreferences: {
      contextIsolation: true,
      sandbox: false,
      preload: path.resolve("preload.mjs"),
    },
  });

  win.loadURL("http://localhost:8080");
})();

preload.mjs

console.log("preload loaded");

resulting in the following output in devtools/console:

image

@Arkellys
Copy link
Author

Arkellys commented Jan 10, 2024

I had the time to search a bit more about this issue, and I managed to pinpoint some elements that trigger it:

  • I can't reproduce the bug on a basic Electron project that doesn't use a bunder.
  • When using webpack as bundler, the bug can be reproduced inconsistently by importing files into the preload. The greater the number of nested files imported, the more likely the bug will appear. I can also always reproduce it by importing an external library like rimraf. I don't know why rimraf in particular though... I have not tried with other libraries.
  • When using Vite as bundler, the bug is even easier to reproduce, it happens inconsistently with a single file import (no nesting needed), and all the time with rimraf.

Test cases:

🟢 No bug - 🔴 Always bug - 🟡 Inconsistent

Action Imports Results (webpack) Results (vite)
Starting the app none 🟢 🟡
files 🔴 🔴
rimraf 🔴 🔴
Reload none 🔴 🔴
files 🔴 🔴
rimraf 🔴 🔴
Force reload none 🟢 🟡
files 🟢 🔴
rimraf 🔴 🔴
Hot reload none 🟢 🟡
files 🟡 🟡
rimraf 🔴 🔴

So yeah, very inconsistent bug... Sadly it's a blocker for switching an app to ESM.

@jkleinsc jkleinsc added the has-repro-comment Issue has repro in comments label Jan 10, 2024
@electron-issue-triage
Copy link

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!

@Arkellys
Copy link
Author

Bump.

@Arkellys
Copy link
Author

Arkellys commented Jul 7, 2024

Issue is still present on v31.1.0.

@RitvarsZ
Copy link

Bump, issue still present on v31.2.1.

@Corvus278
Copy link

Corvus278 commented Sep 17, 2024

Bump, issue still present on v32.1.0.

@KillerCodeMonkey
Copy link

i am using electron v32.x and electron vite with esm.
May renderer part is a react app.

I worked around it my initialize my app on "dev" mode (i am just checking if import.meta.url ends with .tsx with an ugly timeout of half a second and hope that this is enough that the preload script is loaded.

Since i have some component imports that are directly using some apis definded in the preload.mts and need to load them with async imports.

But at least this allows me to stay with wonderful esm.

I guess people using other spa/ui frameworks can work around the same way.
Since it only occurs in dev mode.

@Milkshiift
Copy link

Bump, still an issue in v33.0.2

@Milkshiift
Copy link

Adding a disable-http-cache switch seems to be a workaround and works for my case.
Add this line before the app ready event: app.commandLine.appendSwitch("disable-http-cache");

@YeSilin
Copy link

YeSilin commented Dec 4, 2024

Bump, issue still present on v33.2.1.

@YeSilin
Copy link

YeSilin commented Dec 4, 2024

One solution is to wait for the API provided by the preload script to load completely before calling it, regardless of the execution order, to avoid errors.
一种解决方法是,无论预加载脚本的执行顺序如何,只需等待其提供的 API 加载完成后再调用即可避免报错。

renderer.js

function checkPreloadReady() {
  if (window.api) {
    console.log("Preload script completed. Executing renderer script...");
    // Execute the rendering script code here 在这里执行渲染脚本的代码



  } else {
    console.log("Waiting for preload script to complete...");
    setTimeout(checkPreloadReady, 1000); // Adjust polling interval as needed 轮询间隔时间可以根据需要调整
  }
}

checkPreloadReady();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 has-repro-comment Issue has repro in comments
Projects
None yet
Development

No branches or pull requests

10 participants