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

fix: add workaround for chrome bug 816121 #42

Merged
merged 3 commits into from
Jan 27, 2024
Merged

fix: add workaround for chrome bug 816121 #42

merged 3 commits into from
Jan 27, 2024

Conversation

Jack-Works
Copy link
Member

a dirty hack to workaround Chrome bug https://bugs.chromium.org/p/chromium/issues/detail?id=816121

this dirty hack tries to detect this bug and fix it by calling the classic loader, therefore you must have the correct permissions (like "scripting" in mv3) and host_permissions of the target origin.

you can see the document of classic loader in the readme of this project.

@fregante
Copy link
Contributor

Thank you for the week fix! I'll try it tomorrow or early this week

@fregante
Copy link
Contributor

Screenshot 1

@Jack-Works
Copy link
Member Author

yes, you need to specify a background entry (background page in mv2 or service worker in mv3) in the config to use the classic loader @fregante

@fregante
Copy link
Contributor

fregante commented Jan 19, 2024

I tried: pixiebrix/pixiebrix-extension#7374

MV2 manifest found at https://github.com/pixiebrix/pixiebrix-extension/blob/bb55e53ec263d4d501341775364d4d0291f14927/src/manifest.json

The error disappeared, but the original chunk error returned

Screenshot 13

However I saw this error in the background, I think you're just missing matchAboutBlank: true in your executeScript, we already have the <all_urls> permission.

Screenshot 14

@fregante
Copy link
Contributor

fregante commented Jan 19, 2024

I think you're just missing matchAboutBlank: true

-     const details = { frameId: sender.frameId, file }
+     const details = { frameId: sender.frameId, file, matchAboutBlank: true }
      if (isBrowser) {
        runtime.tabs.executeScript(sender.tab.id, details).then(sendResponse)
      } else {
        runtime.tabs.executeScript(sender.tab.id, details, sendResponse)
      }

Yep, that fixes it :)

Screenshot 18

contentScriptCore is the first chunk to load.

@fregante
Copy link
Contributor

I'm seeing this in the background though, which breaks it for the rest of the extension:

Screenshot 20 Screenshot 21

@Jack-Works
Copy link
Member Author

hello, I cannot reproduce. It works on my side. Here is my setup.

webpack.config.js

const WebExtension = require('webpack-target-webextension')
const webpack = require('webpack')
const { join } = require('path')

/** @returns {webpack.Configuration} */
const config = (a, env) => ({
  module: {
    rules: [],
  },
  entry: {
    bg: join(__dirname, './bg-src.js'),
    cs: join(__dirname, './cs-src.js'),
  },
  output: {
    path: join(__dirname, './dist'),
    publicPath: '/dist/',
    environment: {
      dynamicImport: true,
    },
  },
  plugins: [
    new WebExtension({
      background: { serviceWorkerEntry: 'bg' },
    }),
  ].filter(Boolean),
  devServer: {},
  optimization: {
    minimize: false,
  },
})
module.exports = config

bg-src.js

setTimeout(() => {
  import('./secondary.js')
}, 200)

cs-src.js

if (location.href.startsWith('about')) {
  console.log(location.href)
  import('./secondary.js')
  import('./secondary-2.js')
}

manifest.json

{
  "name": "sandbox iframe",
  "version": "1.0.7",
  "manifest_version": 3,
  "description": "Demo extension",
  "permissions": ["scripting", "activeTab"],
  "host_permissions": ["http://127.0.0.1:8080/*"],
  "background": {
    "service_worker": "dist/bg.js"
  },
  "web_accessible_resources": [
    {
      "resources": ["*.js"],
      "matches": ["<all_urls>"]
    }
  ],
  "content_scripts": [
    {
      "matches": ["*://*/*"],
      "js": ["dist/cs.js"],
      "all_frames": true,
      "match_about_blank": true
    }
  ]
}

secondary.js

console.log('secondary')

secondary-2.js

console.log('secondary 2')

test.html

<html>
<body>
  <!-- srcdoc or src, same issue-->
  <iframe srcdoc="Hello world!" sandbox>
</body>
</html>

@fregante
Copy link
Contributor

Could it be because it's a nested import()? i.e. a import()'d module imports another module.

I'll try again today

@fregante
Copy link
Contributor

fregante commented Jan 26, 2024

Ok I think I found the issue:

new WebExtensionTarget({
  weakRuntimeCheck: true,
  background: {
    pageEntry: "background",
    serviceWorkerEntry: "background",
  },
}),

My build can generate either MV2 or MV3, depending on the MV env, so I set both to the same entry name. However that caused worker code to appear in the background page (hence the importScripts not defined error)

This fixed the background page error already:

new WebExtensionTarget({
  weakRuntimeCheck: true,
  background: {
    pageEntry: "background",
  },
}),

@fregante
Copy link
Contributor

I can confirm that the MV2 version appears to work perfectly, however the MV3 version seems to conflict with mini-css-extract-plugin, in the background worker:

Screenshot

I don't know if this is an issue with this PR or if it's due to the addition of serviceWorkerEntry to the config.

I'll keep testing, but if you want to see it yourself:

# check out https://github.com/pixiebrix/pixiebrix-extension/pull/7374
npm install
MV=3 npm run watch:webpack # or build:webpack, slower
# then run the extension from dist/

@fregante
Copy link
Contributor

fregante commented Jan 26, 2024

Ok I think that issue already existed with serviceWorkerEntry: "background", so this PR is done and ready 🥳

I'll look into the to that loadStylesheet error and open an issue.

@fregante
Copy link
Contributor

BTW, if you want to test frames, srcdoc and sandbox, here you'll find plenty of composable links:

https://github.com/fregante/ephiframe

In this specific case, this link loads both a sandboxed and a non-sandboxed src iframes:

@Jack-Works
Copy link
Member Author

My build can generate either MV2 or MV3, depending on the MV env, so I set both to the same entry name. However that caused worker code to appear in the background page (hence the importScripts not defined error)

I didn't expect pageEntry and serviceWorkerEntry can be the same. I'll investigate this later, try to make it work, and if not, give a warning in this case.

@Jack-Works Jack-Works marked this pull request as ready for review January 27, 2024 14:02
@fregante
Copy link
Contributor

Sounds good, it could be done separately since it's unrelated to frames/loading.

But this is working well for us already. ✅

@Jack-Works Jack-Works merged commit 3f8496b into master Jan 27, 2024
4 checks passed
@Jack-Works Jack-Works deleted the i41 branch January 27, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants