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

The content script is injected twice (editor open, click browser action) #3510

Closed
2 tasks
fregante opened this issue May 29, 2022 · 7 comments · Fixed by #3718 or #3915
Closed
2 tasks

The content script is injected twice (editor open, click browser action) #3510

fregante opened this issue May 29, 2022 · 7 comments · Fixed by #3718 or #3915
Assignees
Labels
bug Something isn't working runtime

Comments

@fregante
Copy link
Collaborator

This bug caused the behavior described in #3132 (comment)

Most of the content script already has duplicate avoidance, but that doesn't seem enough for the QuickBar.

Steps to reproduce

  1. Open new website without permissions
  2. Open editor
  3. Open PB sidebar (enables activeTab and the editor)

Demo

You can see the VM_number prepended to the content script logs, which suggests another contentScript.js was injected.

Screen.Recording.15.mov

Causes

  • The click calls ensureContentScript directly
  • The click also notifies the pageEditor, which also calls ensureContentScript

Maybe next

  • Figure out why the ready/installed test isn't working
  • Add option to ensureContentScript so that it doesn't attempt to inject via this code path
@fregante
Copy link
Collaborator Author

Additionally: While testing I also sporadically saw the same behavior even without the editor open, which is a little concerning.

@twschiller
Copy link
Contributor

twschiller commented Jun 13, 2022

Moving out of 1.7.0 because it mainly impacts the Page Editor

Additionally: While testing I also sporadically saw the same behavior even without the editor open, which is a little concerning.

That's more concerning because it can lead to messenger problems, e.g. possibly #3560

@fregante
Copy link
Collaborator Author

  • Figure out why the ready/installed test isn't working

My guess is that Symbol is not reliable in this scenario and we need to move to exclusively use a Dom-based approach like we do for ready:

document.documentElement.setAttribute(PIXIEBRIX_READY_ATTRIBUTE, "");

This is how I've been tracking duplicate injections in Refined GitHub: https://github.com/refined-github/refined-github/blob/a7c9dd3a0fe6f6ce22b16c8c4f31c7beefdac4e1/source/features/index.tsx#L124-L141

@twschiller
Copy link
Contributor

twschiller commented Jun 14, 2022

My guess is that Symbol is not reliable in this scenario

That's a good theory. If the two scripts run in different VMs, IIRC the symbol would not be equivalent

@twschiller
Copy link
Contributor

twschiller commented Jun 15, 2022

With the different VMs you have to be careful that it's not referring to multiple frames on the page

@fregante
Copy link
Collaborator Author

Reopening this because it wasn't solved and because I'm seeing an additional issue likely caused by this:

  1. Open new website without permissions
  2. Open editor
  3. Click on context menu item that opens the sidebar (show-sidebar is enough)

No context menu found for extension

Screen Shot

@fregante
Copy link
Collaborator Author

I think I'll close this ticked after doing this:

  • Add option to ensureContentScript so that it doesn't attempt to inject via this code path

But in the meantime my other PR fixes the issue in my last comment:

Screen.Recording.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working runtime
Projects
None yet
2 participants