pvrs12 / Anesidora

Anesidora - Pandora extension for Firefox
Other
31 stars 8 forks source link

Anedidora crashes after 2+ hours #76

Closed pvrs12 closed 3 years ago

pvrs12 commented 3 years ago

@lovelydumpling lovelydumpling commented


Edit: Whoops, nevermind. As of this morning it won't play music anymore. Just in Chrome, Firefox (v2.1) still working fine.

Edit 2: Firefox also broke. It seems after an hour or two, both will stop playing songs. Restarting the browser will allow them to play songs again.

@hucario hucario commented yesterday

That's very odd.

Well, I guess it's time for me to listen to anesidora for two hours to try to reproduce this. What a shame, I hate listening to music 😤

@lovelydumpling lovelydumpling commented 15 hours ago •

Sorry, I tend to listen to music for all-nighters due to my sleep habits, or occasionally when reading books/comics/manga. I've only produced it twice, once each. One with Brave overnight, the other with Firefox while reading manga. It seems at least if I'm not listening through such a long stretch, I can stop and start it up just fine. Makes me wonder if Pandora's original "Are you still listening?" behavior has anything to do with it.

Er, anyway, I'm off to bed again so I'll reply or edit this post in the morning whether it happened again on Brave.

EDIT: Yep sure enough as of this morning its broken again. So it does seem to be reproduceable on my end.

@hucario hucario commented 2 hours ago

The only thing I can think of is the callback arrays being a memory leak (whenever you re/open a popup, it adds a function to each array and if you open and close it a bunch it could be a leak? Maybe?) which would lag the player a ton. How does it stop working? Does it hang or crash?

Moved from #68

hucario commented 3 years ago

the only thing I can think to fix it would be adding a callbacks.updatePlayer.splice(i, 1); to the catch block of the try/catch in the forEach https://github.com/pvrs12/Anesidora/blob/a24ce8ccdcc8e212057dd8d6832445f9d979528e/common/js/background.js#L155 https://github.com/pvrs12/Anesidora/blob/a24ce8ccdcc8e212057dd8d6832445f9d979528e/common/js/background.js#L195 https://github.com/pvrs12/Anesidora/blob/a24ce8ccdcc8e212057dd8d6832445f9d979528e/common/js/background.js#L209

->

callbacks.updatePlayer.forEach((e, i) => {
    try {
        e();
    } catch(b) {
        callbacks.updatePlayer.splice(i, 1);
    }
});
pvrs12 commented 3 years ago

@hucario it seems likely that the changes tosetCallbacks is the issue here.

What's the reasoning behind making these lists and adding to them each time the popup is rendered vs the single instance of the callback?

Edit: also, what's the difference with spliceing the list and just keeping it as a single value?

hucario commented 3 years ago

@hucario it seems likely that the changes tosetCallbacks is the issue here.

What's the reasoning behind making these lists and adding to them each time the popup is rendered vs the single instance of the callback?

Often I keep a tab open with the player in it: image

With the old system, when I opened the popup (often by accident trying to get to a different extension's popup) it would replace the callbacks, thus making the open tab out of date

also, what's the difference with spliceing the list and just keeping it as a single value?

splicing the list gets rid of functions that error- which are the functions of popups that were closed: image

doing it like that keeps it as an array, which makes it possible to have multiple tabs / versions open without any of them going out of date, while keeping it as a single value keeps it as a single player

sure, it's an edge use case, but I didn't see any issues in implementing it

if it is causing the leak (which is possible, although unlikely as you would have to open/close the popup around ~1000 times by my estimate in order for it to cause a problem) and the fix doesn't do anything (which is unlikely if it was causing the leak) then sure, reimplement the single single value

but I like having my tab open

image image

lovelydumpling commented 3 years ago

It doesn't hang as far as I can tell? Or at least, it doesn't freeze.

What happens is the player will show it stopped at the end of the last song it played. Then upon trying to play, skip to the next song, load another station, etc. the app just doesn't do anything. It will go to the "Play or resume last station" screen, but other than that nothing will play music again. The buttons all still highlight on mouseover, and I can go to the stations menu as well as whatever the lefthand menu is, in both old and new forms, but it won't start playing any music unless I restart the browser.

pvrs12 commented 3 years ago

What happens is the player will show it stopped at the end of the last song it played. Then upon trying to play, skip to the next song, load another station, etc. the app just doesn't do anything. It will go to the "Play or resume last station" screen, but other than that nothing will play music again. The buttons all still highlight on mouseover, and I can go to the stations menu as well as whatever the lefthand menu is, in both old and new forms, but it won't start playing any music unless I restart the browser.

I suspect that the auth tokens may be expiring. I don't remember offhand when that's refreshed

hucario commented 3 years ago

That sounds like the login token expired.

Ah, now I think of it it's probably the anti-loop pattern I put in. (This was while requests were still synchronous and would hang the main thread, so I just made it so it would retry once.) Here's what's happening (I think):

  1. Login works ( dontRetryPartnerLogin === false)
  2. After a while, token expires ( dontRetryPartnerLogin === false, so tries again but sets dontRetryParnerLogin to true)
  3. login succeeds ( dontRetryPartnerLogin === true)
  4. after a while, token expires again
  5. dontRetryPartnerLogin === true, so it doesn't retry

I'll just make the code set dontRetryPartnerLogin back to false after login succeeds.

pvrs12 commented 3 years ago

I guess this one closed too... Will wait for confirmation from @lovelydumpling

lovelydumpling commented 3 years ago

Got the new version installed, will let you know by tomorrow how it goes :)

hucario commented 3 years ago

by the way, the loop the bug was trying to fix was this:

  1. Attempt login
  2. Partner login succeeds
  3. User login fails
  4. sendRequest sees this and tries again (which is good for some things)
  5. Partner login succeeds
  6. User login fails (despite having a good username/password, because the API change)
  7. sendRequest sees this and tries again :(
  8. Partner login succeeds
  9. by this point it's been 15 seconds with all the UI frozen and people start to submit bug reports

...

  1. chrome realizes something's wrong and yeets the thread; ui unfreezes but nothing is accomplished
  2. User sees that the UI is unfrozen and decides "Hey! I'm going to press the button again!"
lovelydumpling commented 3 years ago

I am able to play music this morning. It was still stopped at the end of whatever last song it was on, and the Skip/Play button didn't work, but clicking off the player and re-opening it and clicking Play/Skip started a new song, so I didn't have to restart the entire browser.

Speaking of, there's a possibly related but distinct (minor) issue, I think it needs its own page so I'll start it: #80

Edit: Gonna test again tonight with 2.2.0 now that the other issues have been resolved.

Edit 2: Mmk, the behavior is mostly unchanged. Where it's at right now:

I wake up, there's no music playing. I open the player, the last song that played is stopped at its end. I press play, nothing happens. I press Skip, it skips to another song, but it never starts playing. I close the player and reopen it, it has no song up. I click play once, nothing happens. I click it again, and it plays a new song.

Bolongna commented 3 years ago

Having the same issue here. Plays fine for just about 2 hours then stops. If I refresh chrome and hit play it starts playing again sometimes, other times it requires closing chrome out, then opening it and selecting anedidora again. Then it plays. This is on both the new and old interfaces. Other than that it seems to work OK.

lovelydumpling commented 3 years ago

Having the same issue here. Plays fine for just about 2 hours then stops. If I refresh chrome and hit play it starts playing again sometimes, other times it requires closing chrome out, then opening it and selecting anedidora again. Then it plays. This is on both the new and old interfaces. Other than that it seems to work OK.

To be clear, have you updated Anesidora to the latest version (2.2.x)? Because your issue sounds like the old original version of the issue from 2.1. As it stands now, music may stop after a while but we no longer have to restart or refresh Chrome to play again, but rather click play/skip, close and re-open the anesidora panel and click play/skip once or twice more.

Bolongna commented 3 years ago

Yes that is with 2.2.1 running on the latest version of Chrome.

lovelydumpling commented 3 years ago

Strange. I'm on 2.2.0, I'll update to 2.2.1 and see if I can reproduce the issue.

Edit: So far I'm unable to reproduce the issue, I don't have to restart/refresh the browser to be able to play music again. Perhaps there's something different between Chrome and Chromium browsers like Brave (which is what I use)?

Bolongna commented 3 years ago

Possibly. Google likes to play with everything all the time.

Bolongna commented 3 years ago

OK, after a couple days it is now working somewhat better, it still stops at around 2 hours, but now I can simply hit play a couple times or re-open the panel to get it working again.

lovelydumpling commented 3 years ago

Alright that's about where I'm at too.

Bolongna commented 3 years ago

OK if I'm correct it was something outside the extension, at least on mine it is now playing for at least 5 hours non-stop. I noticed that last night I started it around 01:00 and dozed off, woke up at 06:15 and it was still playing with no intervention.

lovelydumpling commented 3 years ago

OK if I'm correct it was something outside the extension, at least on mine it is now playing for at least 5 hours non-stop. I noticed that last night I started it around 01:00 and dozed off, woke up at 06:15 and it was still playing with no intervention.

I'm still having the issue. Wish I knew what fixed it for you.

lovelydumpling commented 3 years ago

Fixed by #94 ! It lasted the full night. About 10 hours. Should be able to close this issue now. (Although perhaps not until it's fixed in the Release.)

pvrs12 commented 3 years ago

Released with https://github.com/pvrs12/Anesidora/releases/tag/v2.2.3

Bolongna commented 3 years ago

Working like it used to now, doesn't stop even after 20+ hours. OH and FYI it will also run on the Edge browser ! Tried it as a test. Then removed it and edge.... Thanks folks.