Attempt to make frontend-to-addon comms more robust #1102
Conversation
|
I still can't reliably reproduce this issue - and I have never been able to do so on local development. It happens for me like 1 out of 20 attempts on staging. My strong hunch is that this is a race condition, but I don't know where the race is happening. So, this PR attempts to stop the number of things happening at once and retry things a few times before reporting an error. And then, on error, show the error page
|
| }); | ||
| }, | ||
|
|
||
| // Send webChannel message to addon, use a Promise to wait for the answer. | ||
| waitForMessage(type, data, timeout = 10000) { | ||
| waitForMessage(type, data, timeout = 5000, retries = 3) { |
There was a problem hiding this comment.
FWIW, reduced timeout to 5 seconds from 10 - because the retries should actually lead to a total timeout of 15 seconds before rejection.
|
@meandavejustice @6a68 @chuckharmston - this could use some additional eyes and sanity checking, because I'm kind of handwaving at the problem here 😿 |
|
@ckprice - oh and hey, if you can try this out, it could be helpful since you reported being able to reliably reproduce this! |
I'm not a fan of this; it could lead to a loop of endless refreshes if the user is like @ckprice and can reproduce reliably. Maybe ship them to the error page, instead? |
|
Otherwise, code looks good and logic is sound. Unfortunately, like you I can't reproduce the original issue, so I'm reticent to merge anything. I'll let someone else check it out. |
- Perform initial experiments API fetch and add-on installed experiment message happen in series, rather than in parallel - `app.waitForMessage` will now retry several times before finally timing out with a rejection (default 3 times) - If all else fails on app init, bounce to error page instead of blank white page Fixes mozilla#1101
lmorchard
force-pushed
the
1101-more-robust-addon-messaging
branch
from
3b2a286
to
4d033c0
Compare
Yeah, the more I think about that, the less I like it. Just pushed a tweak to bounce to the error view, rather than reload. Better than a blank white page, at least. |
| app.pageManager = new PageManager({ | ||
| pageContainer: document.querySelector('[data-hook=page-container]') | ||
| }); | ||
| if (!app.router.history.started()) { |
There was a problem hiding this comment.
Also, moving page manager & router init outside of the experiments fetch means we can show the error page.
Side effect will be that the page will start to render before the API actually returns results, so typically a sub-second lag until filling in with experiments. Previously, the whole page would just stay white until the experiments came in.
Pulled this down locally and was unable to trigger the error. My "pretty reliable" STR's on production have become less reliable in the past couple of hours FYI. One in every 10-12 tries or so. |
|
Unless there are objections, I may end up self-merging this tomorrow just to get it in there for next deploy. |
|
@lmorchard pulling this down this morning for a quick check then I'm fine to merge for you |
message happen in series, rather than in parallel
app.waitForMessagewill now retry several times before finallytiming out with a rejection (default 3 times)
white page
Fixes #1101