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

Main world injection: Prevent race condition #1520

Merged

Conversation

marcustyphoon
Copy link
Collaborator

@marcustyphoon marcustyphoon commented Jun 23, 2024

Description

#1514 relies on the fact that fetching a json file and performing an extension storage query will take longer than the browser fetching the main_world script file, as we need the latter to be performed before importing modules with side effects or executing code that uses injection.

This performs a back-and-forth handshake to confirm that the main_world script file has been executed before proceeding with main install.

Random aside: to improve extension load speed slightly, one can do:

    const [
      installedScripts,
      { enabledScripts = [] }
    ] = await Promise.all([
      getInstalledScripts(),
      browser.storage.local.get('enabledScripts'),
      mainWorldReady
    ]);

but I can't figure out how to make the code not look stupid.

Testing steps

  • Confirm basic extension functionality.
  • If desired, unrevert the test commit and confirm that basic functionality appears with a significant delay on page load.

I have no tests for the bad case this prevents, as I can't for the life of me get it to happen.

@marcustyphoon marcustyphoon added the bugfix Restores intended behavior label Jun 23, 2024
@AprilSylph
Copy link
Owner

but I can't figure out how to make the code not look stupid.

I don't think it looks that stupid, and it serves a clear purpose of "do these things in parallel instead of sequentially".
I've also seen the exact same pattern in the Redpop codebase.

@marcustyphoon
Copy link
Collaborator Author

marcustyphoon commented Jul 1, 2024

Yeah, I mean, it's definitely the way that you do that. I just wish the inputs and outputs lined up... and that there wasn't a mismatch in the inputs and outputs in this case.

(Thought: I guess in a typescript codebase it's less important to make it visually clear in code what expressions correspond with one another, because you'll get type errors immediately if you change things in an erroneous way. Vanilla javascript imposes more discipline in that regard, which can be inconvenient but can also be good practice!)

@marcustyphoon marcustyphoon merged commit 8c574ab into AprilSylph:master Jul 13, 2024
2 checks passed
@marcustyphoon marcustyphoon deleted the injection-race-condition branch July 13, 2024 18:57
marcustyphoon added a commit to marcustyphoon/XKit-Rewritten that referenced this pull request Jul 13, 2024
marcustyphoon added a commit to marcustyphoon/XKit-Rewritten that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Restores intended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants