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

Check every API request for correct content type #1257

Closed
kolaente opened this issue Oct 17, 2022 · 26 comments
Closed

Check every API request for correct content type #1257

kolaente opened this issue Oct 17, 2022 · 26 comments
Labels
bug Something isn't working investigation-required The Issue or PR needs more in-depth analysis stalebot-enabled The PR/issue is marked to go stale over time

Comments

@kolaente
Copy link

Description
Opening the cookbook app in a browser shows a loading spinner at "categories", does not seem to load anything (from the network tab and completely freezes the browser to the point where Firefox would offer to kill the tab. This looks like it is running into some kind of endless loop.

Reproduction
Steps to reproduce the behavior:

  1. Open the cookbook app in the browser

Expected behavior
App working normally like it used to.

Actual behavior
Not working, locks up browser

Screenshots

This is what that looks like when it locks up:

image

Browser

Tested in Firefox 104.0.2 and Chromium 105.0.5195.125, both on Linux (NixOS).

Versions
Nextcloud server version: 24.0.1
Cookbook version: 0.9.15
Database system: MariaDB

@kolaente kolaente added the bug Something isn't working label Oct 17, 2022
@christianlupus christianlupus added this to the Release 0.9.16 milestone Oct 18, 2022
@christianlupus
Copy link
Collaborator

Can you please open the developer tools before navigating to the cookbook app? There should be a few requests running. Potentially, the JS is not loading successfully. Could you please check this?

@christianlupus christianlupus added investigation-required The Issue or PR needs more in-depth analysis stalebot-enabled The PR/issue is marked to go stale over time labels Oct 18, 2022
@kolaente
Copy link
Author

It looks like all assets are loaded successfully.

@MarcelRobitaille
Copy link
Collaborator

Have you checked specifically for cookbook-main.js? Nextcloud itself loads many assets outside of this app, so it may not be obvious whether this app's assets are loaded. In the network tab, you can filter for cookbook, which for me shows that cookbook-main.js is being loaded (I have highlighted it in the screenshot).

image

@kolaente
Copy link
Author

@MarcelRobitaille Yes that file gets loaded.

@MarcelRobitaille
Copy link
Collaborator

@kolaente Are there any errors in the console?

@kolaente
Copy link
Author

Only CSP warnings, not sure if they are related:

image

@MarcelRobitaille
Copy link
Collaborator

@kolaente Thanks. No, I also always get those. I don't think it's that.

Not working, locks up browser

What do you mean by this? Does the entire browser become unresponsive? Are other tabs and the browser's buttons also misbehaving / responding slowly?

@kolaente
Copy link
Author

Does the entire browser become unresponsive? Are other tabs and the browser's buttons also misbehaving / responding slowly?

Other tabs get slowed down a bit but are still usable. The tab with the cookbook open is completely unresponsive. Firefox uses two cpu cores at 100% during the unresponsiveness.
Firefox offers me to stop the tab after a few minutes:

image

@MarcelRobitaille
Copy link
Collaborator

Do you think you could go the the "Performance" tab of the developer tools and take a profiler recording? That could help us figure out what the page is stuck doing.

@kolaente
Copy link
Author

Here's a performance record: https://share.firefox.dev/3s9oryG

At ~3.5 sec mark the page was loaded and started to freeze.

@MarcelRobitaille
Copy link
Collaborator

@kolaente Thanks very much for that. I hoped there would be a little more information about the calls, but alas. I looks like getCategories is being called repeatedly from 4.75s to 6.25s, then all the time is spent in totalRecipeCount.

@christianlupus Do you have a proposal of how to debug this? My instinct would be to have @kolaente try a new branch with some console.log thrown in, but maybe this is an opportunity to add some more permanent logging?

@kolaente
Copy link
Author

@MarcelRobitaille These two functions look like they have potential for an infinite loop. Would you accept a PR with a refactor to forEach or similar?

@MarcelRobitaille
Copy link
Collaborator

@kolaente I don't see how either of these could create an infinite loop. The loop variables are not being modified in the body of the loop. Furthermore, getCategories is being called multiple times, it's not being called once and getting stuck. I proposed the logging so we could wee from where getCategories is being called.

@christianlupus
Copy link
Collaborator

If you open the debugger console before you navigate to the cookbook app and switch to the network tab, you should see all requests that are carried out by the frontend. Is the number when opening the cookbook app flowing through like an avalanche? Are the requests finished at some time (return code in the first column appears)?

Did the app work once?

Apart from that, I think the problem is most probably in the front end and I suspect some nasty thing there. I have it running with a very similar configuration and I do not see any problems.

Related to your question about debugging: I was considering adding a boolean flag that will allow the user to temporarily enable debugging in the browser console. Something like a checkbox in the settings 😉 that is not stored and thus reset to false in case of reloading the page.

I fear this is not a good solution in itself here as it is not possible to activate that before the user can visit the main page. It might however be a good idea to implement the logging and create a branch where the logging is enabled by default. Then, @kolaente can test using this branch.

What do you think of this idea, @MarcelRobitaille?

@christianlupus christianlupus removed this from the Release 0.9.16 milestone Oct 20, 2022
@MarcelRobitaille
Copy link
Collaborator

@christianlupus Yes, I was thinking something similar. If we want to see output in the console, and if the settings UI hasn't loaded yet, what if we have the user run a command in the console to enable logging before navigating to the cookbook app? We could use localstorage. Maybe we could make it reset automatically after a certain time?

@christianlupus
Copy link
Collaborator

christianlupus commented Oct 20, 2022

That sounds very reasonable. I would let that to you, if you do not mind. You are way more experienced in js than me. If you want, you can add another issue to keep track of progress.

I will try to prepare the nc25 release soon. So, please be prepared to rebase if you start to implement things.

@kolaente
Copy link
Author

If you open the debugger console before you navigate to the cookbook app and switch to the network tab, you should see all requests that are carried out by the frontend. Is the number when opening the cookbook app flowing through like an avalanche? Are the requests finished at some time (return code in the first column appears)?

There's a bunch of requests happening intially for ~1-3 seconds. After that the list stays consistent at 71 requests.

Did the app work once?

Yes. Right now that's my biggest problem because I have quite a few recipes in there and can't open them now.


While looking at the network tab I saw something else: The requests to /apps/cookbook/webapp/categories, /apps/cookbook/webapp/config and /apps/cookbook/webapp/recipes all return a 302 and redirect to the dashboard at /apps/dashboard/. I assume those are the calls to get everything as json?

@christianlupus
Copy link
Collaborator

christianlupus commented Oct 21, 2022

There's a bunch of requests happening intially for ~1-3 seconds. After that the list stays consistent at 71 requests.

At least we are not facing an infinite loop, I was fearing.

Yes. Right now that's my biggest problem because I have quite a few recipes in there and can't open them now.

The good news is: Your recipes are safe, even from yourself 😉. No, to be serious: The recipes are stored as files and can be accessed anytime you like.

While looking at the network tab I saw something else: The requests to /apps/cookbook/webapp/categories, /apps/cookbook/webapp/config and /apps/cookbook/webapp/recipes all return a 302 and redirect to the dashboard at /apps/dashboard/. I assume those are the calls to get everything as json?

Yes, that sounds strange. These should return JSON data. When HTML data (with the redirect) is used and parsed as JSON, that might cause some hiccups. So, there is for sure some problem with the backend so far.

Do you see any errors in the nextcloud.log file appearing?

You could try to disable and reenable the cookbook app once in the backend to make sure, it is registered accordingly.

If you are familiar with curl, you can trigger the command to get the categories by

curl https://example.com/apps/cookbook/api/v1/categories -v -u user:user_pwd

Let's see what happens then. Just post the output here carefully (redact any data in a header mentioning cookies or authorization!)

@kolaente
Copy link
Author

I tried disabling and re-enabling the app with no luck. Then I noticed my instance was still on 24.0.1 so I upgraded it to 24.0.6... and now everything works as expected. Looks like this wasn't an issue in the cookbook app after all. It's probably still a good idea to add a check before looping over the json data to avoid these loops.

@christianlupus christianlupus changed the title Opening the cookbook freezes browser, makes the app unusable Check every API request for correct content type Oct 23, 2022
@MarcelRobitaille
Copy link
Collaborator

@christianlupus Would #1283 be helpful here or have we moved passed that?

@christianlupus
Copy link
Collaborator

The problem is solved for the thread starter. So, this is done.

I would suggest adding a generic log to the API Axios request generator. Something that logs basic information about all requests carried out. What are you thinking about that, @MarcelRobitaille?

@christianlupus
Copy link
Collaborator

PS: Also, all requests could be assigned a check if they contain application/json or text/json and issue a warning if not (possibly configurable).

@MarcelRobitaille
Copy link
Collaborator

@christianlupus
Copy link
Collaborator

Yeah, sort of. I might have realized it differently but the main point it is.

@MarcelRobitaille
Copy link
Collaborator

I am open to suggestions. What would you have changed? After that, shall I create a pull request?

@christianlupus
Copy link
Collaborator

All right. I was not aware of the option to incept. Open a PR and we never this directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigation-required The Issue or PR needs more in-depth analysis stalebot-enabled The PR/issue is marked to go stale over time
Projects
None yet
Development

No branches or pull requests

3 participants