Endless loading animations for bad URL

[muffinresearch]

See https://addons.mozilla.org/en-US/firefox/addon/%20%7Bc5b32a48-5514-4a46-81f2-075ebf3cbc29%7D/ (via Gijs on Slack)

This should be a 404, or better still the leading space could be trimmed and redirected to a valid GUID URL.

[jfkthame]

Note that a trailing space on the GUID also triggers this effect.

[gijsk]

looks like the page fetches https://addons.mozilla.org/api/v4/addons/addon/%20%7Bc5b32a48-5514-4a46-81f2-075ebf3cbc29%7D/?app=firefox&appversion=81.0&lang=en-US&wrap_outgoing_links=true which 404s, and then it just keeps fetching that over and over, and re-rendering some bits of UI, which leads to the flickering.

[bobsilverberg]

The problem here is twofold:

1. We should trim any string add-on ids before sending an API request, and we're not doing that.
2. We did have code that trimmed the add-on id before checking to see if an add-on was already loading, but because we didn't trim the id in the fetchAddon reducer function, we were unable to recognize that the add-on was already loading, which was what was causing the loop. Trimming both when calling fetchAddon (which will then store the trimmed id), and also when checking to see if an add-on is already loading (via isAddonLoading) will eliminate the cause of this loop.

[willdurand]

@bobsilverberg I suggest we update one of our middleware instead of only fixing one page (add-on detail page). We shouldn't load an URL containing a leading whitespace because that's not correct. Instead, we should redirect to the right URL (i.e. the one without a whitespace). We can probably trim all URL parameters.

[bobsilverberg]

@bobsilverberg I suggest we update one of our middleware instead of only fixing one page (add-on detail page). We shouldn't load an URL containing a leading whitespace because that's not correct. Instead, we should redirect to the right URL (i.e. the one without a whitespace). We can probably trim all URL parameters.

Trimming all URL parameters sounds like a good idea. I'll take a look and see if I can figure out how to do that.

[muffinresearch]

@bobsilverberg I suggest we update one of our middleware instead of only fixing one page (add-on detail page). We shouldn't load an URL containing a leading whitespace because that's not correct. Instead, we should redirect to the right URL (i.e. the one without a whitespace). We can probably trim all URL parameters.

This is a good point, would be good to check if there's a similar problem with other slugs. Though from what I can see from a couple of tests, other pages result in a standard 404 rather than this behaviour:

• https://addons.mozilla.org/en-US/firefox/addon/%20japanese-kimono-motif-343341-2/reviews/
• https://addons.mozilla.org/en-US/firefox/%20language-tools/

[bobsilverberg]

I think the above is explained by the fact that this is a problem with fetchAddon, so other pages, which don't call that, and also don't check that an add-on is currently being loaded, won't suffer from this same problem.

[muffinresearch]

I think the above is explained by the fact that this is a problem with fetchAddon, so other pages, which don't call that, and also don't check that an add-on is currently being loaded, won't suffer from this same problem.

@bobsilverberg I'm in two minds about this, if it isn't actually a problem elsewhere, then do we need to add the overhead of trimming everything between slashes for every single request? As long as other URLS with spaces 404 and don't cause this issue it's not a problem. Feel free to convince me otherwise though.

[bobsilverberg]

I think the above is explained by the fact that this is a problem with fetchAddon, so other pages, which don't call that, and also don't check that an add-on is currently being loaded, won't suffer from this same problem.

@bobsilverberg I'm in two minds about this, if it isn't actually a problem elsewhere, then do we need to add the overhead of trimming everything between slashes for every single request? As long as other URLS with spaces 404 and don't cause this issue it's not a problem. Feel free to convince me otherwise though.

I don't disagree, although it does seem like a worthwhile thing to do, as an overall fix. Considering that this is a P1, and should land today, and @willdurand is off until tomorrow, maybe we should land the fix in https://github.com/mozilla/addons-frontend/pull/9671, and then also consider the middleware approach as a follow-up.

@muffinresearch WDYT?

[bobsilverberg]

I think this needs a bit more thought/discussion. Here are the facts:

1. When you issue an API request for a slug with a trailing space, it works. The server strips the trailing space out at some point, and returns a response for the add-on. Note that this does not work for a leading space, just a trailing space.

2. Our code would expect that issuing a request for a slug that doesn't match the slug of the requested add-on would result in a 404, but, as per 1 above, it does not. This is actually the root of the problem reported in https://github.com/mozilla/addons/issues/12583. I'm not sure the fix in https://github.com/mozilla/addons-frontend/pull/6819 is really the correct one.

3. If the API request returned a 404, as one might expect, then the problem in https://github.com/mozilla/addons/issues/12583 would never have existed, as we'd get a 404 page. We should perhaps consider whether the fix for this issue is to "unfix" the code on the server so it doesn't do anything to the slug it receives, and returns a 404 as expected.

4. As @eviljeff pointed out, the slug is invalid, so rather than trying to trim anything perhaps we should just allow the 404 to happen. The most sensible way to fix that might be the server fix as described in 3, above.

5. If we do want to trim slugs so that any slug with a leading or trailing space work, we need to decide how best to do that. We discussed using some middleware, which does seem like a sensible approach, but it also adds overhead to every request.

Any thoughts anyone has would be appreciated.

@muffinresearch @eviljeff @diox @willdurand

[willdurand]

2\. Our code would expect that issuing a request for a slug that doesn't match the slug of the requested add-on would result in a 404, but, as per 1 above, it does not. This is actually the root of the problem reported in mozilla/addons#12583. I'm not sure the fix in mozilla/addons-frontend#6819 is really the correct one.

The issue was verified fixed so I believe it used to work. Maybe it has regressed somehow?

3\. If the API request returned a 404, as one might expect, then the problem in mozilla/addons#12583 would never have existed, as we'd get a 404 page. We should perhaps consider whether the fix for this issue is to "unfix" the code on the server so it doesn't do anything to the slug it receives, and returns a 404 as expected.

Yeah, it would be interesting to understand why the API does that. Sanitizing URL params make sense but I don't think we should trim anything, so the API should return a 404 instead.

[diox]

The API doesn't do it, MySQL does.

SELECT `addons`.`id` FROM `addons` WHERE `addons`.`slug` = "some-slug "

Returns the add-on with slug some-slug without the space. This is a collation issue, and it sort of makes sense, sort of not. But it's tricky to fix without switching to MySQL 8.0 and changing the collation (which we have planned, but long-term).

[diox]

It's worth pointing out that it's not just the trailing space: by design, the collation we're using will also unfold accents best it can, ignore case and much, much more. So, taking some-slug as the slug again, this will also return it:

SELECT `addons`.`id` FROM `addons` WHERE `addons`.`slug` = "söme-Slug "

[willdurand]

Locally, I am not able to reproduce the issue with the trailing space. The server redirect is performed as expected.

I wrote a test case for the leading space, the test case passes but it does not work in the browser and that's because the API returns a 404 in this case. Here is the test case:

diff --git a/tests/unit/amo/pages/TestAddon.js b/tests/unit/amo/pages/TestAddon.js
index 68cf92a2b..71d947f0d 100644
--- a/tests/unit/amo/pages/TestAddon.js
+++ b/tests/unit/amo/pages/TestAddon.js
@@ -555,6 +555,31 @@ describe(__filename, () => {
     sinon.assert.callCount(fakeDispatch, 1);
   });

+  it('dispatches a server redirect when slug has a leading space', () => {
+    const slug = 'some-slug';
+
+    const clientApp = CLIENT_APP_FIREFOX;
+    const { store } = dispatchClientMetadata({ clientApp });
+    const addon = { ...fakeAddon, slug };
+    store.dispatch(_loadAddonResults({ addon }));
+
+    const fakeDispatch = sinon.spy(store, 'dispatch');
+    renderComponent({
+      // Add a leading space to the slug
+      params: { slug: ` ${slug}` },
+      store,
+    });
+
+    sinon.assert.calledWith(
+      fakeDispatch,
+      sendServerRedirect({
+        status: 301,
+        url: `/en-US/${clientApp}${getAddonURL(addon.slug)}`,
+      }),
+    );
+    sinon.assert.callCount(fakeDispatch, 1);
+  });
+
   it('dispatches a server redirect when slug has trailing spaces', () => {
     const slug = 'some-slug';

I looked at how 404s from the API are handled, and we handle them correctly. That's what we can observe if we load the site without JS enabled (or from curl):

$ curl -I https://addons.mozilla.org/en-US/firefox/addon/%20%7Bc5b32a48-5514-4a46-81f2-075ebf3cbc29%7D/
HTTP/1.1 404 Not Found
[...]

This means something goes wrong after that and looking at the componentDidUpdate(), I noticed that we don't prevent fetchAddon() to be executed indefinitely, which is the reason of the original report. I guess we should have something like this:

diff --git a/src/amo/pages/Addon/index.js b/src/amo/pages/Addon/index.js
index 58483b5ea..2bd41696e 100644
--- a/src/amo/pages/Addon/index.js
+++ b/src/amo/pages/Addon/index.js
@@ -147,6 +147,10 @@ export class AddonBase extends React.Component {
       match: { params },
     } = this.props;

+    if (errorHandler.hasError()) {
+      return;
+    }
+
     const oldAddonType = oldAddon ? oldAddon.type : null;
     if (newAddon && newAddon.type !== oldAddonType) {
       dispatch(setViewContext(newAddon.type));

With this quick patch, we render a 404 and everything looks good. Also, the test suite seems to be happy. We've done that for the homepage in the past, so I guess that would make sense? WDYT?

[willdurand]

It's also worth mentioning that the fix for https://github.com/mozilla/addons/issues/12583 cannot work for trailing spaces because in the case of leading spaces, the API returns a 404. Our code only sends a server redirect when there is no error (a 404 from the API is an error).

[eviljeff]

It's worth pointing out that it's not just the trailing space: by design, the collation we're using will also unfold accents best it can, ignore case and much, much more. So, taking some-slug as the slug again, this will also return it:

SELECT `addons`.`id` FROM `addons` WHERE `addons`.`slug` = "söme-Slug "

The fix for mozilla/addons#12583 seems flawed in light of this - it's not just extra spaces on the end of the slug that would be ignored for a match in the database, it's possibly a whole range of different characters. So we can't wait for an API response that "matches" the slug or guid (unless there's a very loose definition of "match" that is the same as mysql's collation).

I'd split that ^ into an issue that can be addressed at our leisure though and just fix the leading space problem.

[willdurand]

I'd split that ^ into an issue that can be addressed at our leisure though and just fix the leading space problem.

Yes, this is an addon-server problem. The API should probably handle this MySQL weirdness by verifying that the add-on slug is the slug passed in the URL.

[eviljeff]

I'd split that ^ into an issue that can be addressed at our leisure though and just fix the leading space problem.

Yes, this is an addon-server problem. The API should probably handle this MySQL weirdness by verifying that the add-on slug is the slug passed in the URL.

Well, I meant more why do we need to wait for an exact match for the slug/guid anyway? Isn't getting back an add-on detail with a 200 response a sufficient indication that a matching add-on has been found?

[bobsilverberg]

2. Our code would expect that issuing a request for a slug that doesn't match the slug of the requested add-on would result in a 404, but, as per 1 above, it does not. This is actually the root of the problem reported in mozilla/addons#12583. I'm not sure the fix in mozilla/addons-frontend#6819 is really the correct one.

The issue was verified fixed so I believe it used to work. Maybe it has regressed somehow?

I wasn't suggesting that the fix didn't "fix" the specific issue from mozilla/addons#12583, but rather that it's probably not the correct overall fix for the root problem. It's a bandaid that works for that one case.

[bobsilverberg]

It's also worth mentioning that the fix for mozilla/addons#12583 cannot work for trailing spaces because in the case of leading spaces, the API returns a 404. Our code only sends a server redirect when there is no error (a 404 from the API is an error).

Yes, this is the point I've been trying to make, and is why the "fix" for mozilla/addons#12583 isn't really the correct overall fix.

[bobsilverberg]

I'd split that ^ into an issue that can be addressed at our leisure though and just fix the leading space problem.

Yes, this is an addon-server problem. The API should probably handle this MySQL weirdness by verifying that the add-on slug is the slug passed in the URL.

Well, I meant more why do we need to wait for an exact match for the slug/guid anyway? Isn't getting back an add-on detail with a 200 response a sufficient indication that a matching add-on has been found?

We could take that approach. It wouldn't update the URL however, which is part of what our code does. If we get back a result which we think is correct (based on some logic in the component), but the slug on the URL isn't identical to the slug of the add-on, we redirect to the slug for the add-on. This is probably not necessary though. If we simply accept any 200 response that contains an add-on as successful, and just display that data, and don't bother trying to fix the URL, we can probably remove some of this code and make it all work, but I'm not sure if we've decided that we want to do that.

It seems line we need to answer some questions:

1. Should a URL that contains a slug that is not identical to the add-on's slug generate a 404? If we want to take that approach, and make changes to force that to be a 404, then we don't need any frontend code for that (and can possibly even remove some).

2. Should we display an add-on detail page if we receive an API response back, even if the add-on's slug doesn't match the slug on the URL? 2a. If yes, should we redirect, or otherwise fix the URL so that the URL contains the correct slug, or 2b. Can we just leave the URL as is, with an incorrect slug, but still display the add-on detail page?

[willdurand]

I wasn't suggesting that the fix didn't "fix" the specific issue from mozilla/addons#12583, but rather that it's probably not the correct overall fix for the root problem. It's a bandaid that works for that one case.

Exactly, it's a patch we wrote with the information we had at this time. I feel like typing a trailing space might happen in real life, though, and QA didn't find any other similar issue back then (and they usually check other possible scenarios).

1. Should a URL that contains a slug that is not identical to the add-on's slug generate a 404?

I personally think so, yes. That will help us avoid duplicate content in addition to other annoying bugs in the future.

2\. Should we display an add-on detail page if we receive an API response back, even if the add-on's slug doesn't match the slug on the URL?

I don't really understand why we would need to do that. This is clearly a bug in the server code so why not fixing the problem there? If we ever need to support this, we would have to send a redirect because that's what we are doing for numeric IDs and GUIDs (we redirect to the URL with the slug, which is the canonical URL).

Anyway, this issue is about endless loading animations, which can be fixed by preventing fetchAddon to be dispatched all the time in case of an error, as described here: https://github.com/mozilla/addons/issues/13823. Since this is the approach we use in other page-level components, we should likely start with that.

The collation bug is a different issue and we should probably fix it with a different patch (in addons-server, ha! :p) We can probably leave the trailing space fix in place, since it's more likely to add an extra space at the end than in the middle of an URL.

[willdurand]

We discussed about this issue during the standup. We cannot really fix the MySQL weirdness because we sort of rely on it to have case insensitivity and other useful features. We'll have to support weird slugs/GUIDs and we'll send a server redirect to the correct URL (which uses the add-on slug) when slugs/GUIDs are different in the URL vs API response.

That last part can be fixed in a separate issue, though. We can already fix the endless loading animation by preventing indefinite dispatches in case of an error.

[bobsilverberg]

I opened mozilla/addons#13831 as a follow-up for the redirect.

[ioanarusiczki]

@willdurand I verified this on AMO stage but I tried to reproduce the above issue on prod too. The way to reproduce this issue on AMO prod is to add one space in the URL between /addon/ and /guid/ or even /slug/.

Example: https://addons.mozilla.org/en-US/firefox/addon/%20measure-it/?utm_source=addons.mozilla.org&utm_medium=referral&utm_content=featured

AMO prod

On AMO stage this continuous loading state is no longer displayed - it's a 404

[willdurand]

@ioanarusiczki the 404 is indeed expected