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

Incorrect "connection lost" message for multi-page flows #2902

Closed
twschiller opened this issue Mar 8, 2022 · 4 comments · Fixed by #2917
Closed

Incorrect "connection lost" message for multi-page flows #2902

twschiller opened this issue Mar 8, 2022 · 4 comments · Fixed by #2917
Assignees
Labels
bug Something isn't working runtime user experience Improve the user experience (UX)
Milestone

Comments

@twschiller
Copy link
Contributor

twschiller commented Mar 8, 2022

Steps to reproduce

  • Create an extension with a 1) open tab, 2) wait (window: target)
  • Run the extension and close the new target tab while it's waiting

Actual Behavior

  • On the starting tab, PixieBrix will show a non-dismissable connection lost alert

Expected Behavior

  • PixieBrix should show a normal action failed message for the action. (It still has a connection to the host page)

Considerations

  • We should record a more informative error message in this case, such as "Target tab unexpectedly closed"
  • This also affects opener target. If the opener tab is closed, it should be "Opener tab no longer exists", and/or "Opener tab unexpectedly closed

Related code:

@twschiller twschiller added this to the 1.5.7 milestone Mar 8, 2022
@fregante
Copy link
Collaborator

fregante commented Mar 8, 2022

This might be a little involved because with the Messenger we've basically gone towards: "Try messaging this target, until it exists, full stop."

The Messenger doesn't differentiate between a non-existing target and a target that's loading, we tried to make it completely opaque to the caller. Other requirements that came along, like named targets ("tabId-less targets"), also changed how we wait for this to happen and what we allow as an "error" that allows retries.

I'll look into it tomorrow, maybe it's unrelated to all that.

@fregante
Copy link
Collaborator

fregante commented Mar 9, 2022

Uh oh. It's actually a deeper problem. We can't distinguish between:

  • the page unloaded before responding (tab closed or page refreshed)
  • the tab never existed

Here's what happens:

  1. The tab receives the message and the sender awaits a response
  2. You close the tab
  3. Chrome throws "The message port closed before a response was received."
  4. webextension-polyfill purposefully silences this error, resolving the underlying sendMessage with undefined:
  5. webext-messenger treats undefined as a loading error or "the extension has not yet loaded" 🟣
  6. webext-messenger will retry
  7. The tab no longer exists, so we start getting "Could not establish connection. Receiving end does not exist."
  8. The time runs out
  9. webext-messenger throws this last error
  10. The extension treats this error as a complete runtime failure 🟣

The steps marked with 🟣 can likely be improved with little effort. I don't expect the polyfill to fix this issue in the short term or maybe even ever.

@twschiller
Copy link
Contributor Author

twschiller commented Mar 9, 2022

I think your analysis looks correct. I was thinking of just solving the proximate cause:

Can we just wrap the runBrick calls in a try/catch and change the error message? This should be a BusinessError, since when dealing with target/opener/etc. it's generally a misconfigured multipage extension

Then our handlers that look for connection related errors would just see it as a normal error

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 user experience Improve the user experience (UX)
Projects
None yet
2 participants