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

Have to reload extension every few days #157

Closed
brianzam opened this Issue Jun 7, 2017 · 17 comments

Comments

Projects
None yet
2 participants
@brianzam

brianzam commented Jun 7, 2017

Seems like it gets stuck pretty much all the time now and my playlists fail to sync every few days. Reloading sorts it out temporarily but the issue just returns. Appreciate you're probably already working on it Simon but something ain't right :)

I'm using Chrome on a MacBook pro.

Cheers,

Brian

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 7, 2017

Yeah, I've noticed this too: both in my personal use and through the support tickets.

As far as I can tell, it's largely caused by unexpected interactions with Google that the extension doesn't handle well. I haven't had much time for it recently (I'm moving and work has been busy) but I plan to chip away at this throughout the summer.

I've also considered switching from a background page to an event page, which would have the side effect of basically reloading the extension before every interaction. I'm not sure it's a good idea, though, since it'd require either persisting the local cache or reloading it every time.

@simon-weber simon-weber added the bug label Jun 7, 2017

@brianzam

This comment has been minimized.

brianzam commented Jun 7, 2017

Seems like a good idea. Can you branch the code and let some of us try it out?

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 7, 2017

Sure thing, though the event page idea may be tough to test out since I can't change it at runtime. I think I'd either need to do a testing group in the web store or have people sideload the extension. . .

I did have one other idea I may as well note: the extension api has a way for the extension to reload itself. The trouble is that we need a connection to a script in a Google Music tab to finish starting back up, and I don't think I can get that without also refreshing the tab. So, while I originally thought that would be a good bandaid, it boils down to roughly the same amount of work as switching to an event page.

@brianzam

This comment has been minimized.

brianzam commented Jun 8, 2017

You're in the best position to decide what to do, but the events page seems to be a better option from what you've said.

The playlist manger is really useful so I hope we can resolve these issues soon!

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 17, 2017

I just released a version that may help a little bit: it prevents syncing from breaking after deleting a playlist, especially for users with lots of autoplaylists.

No progress on a more robust fix yet, though.

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 18, 2017

I did some research, and I think I have a workable idea: I could ditch the content script entirely and just switch to injecting code. This would remove the need to refresh the page after reloading the extension, meaning I could trigger it from the extension itself.

I think this will work just fine since the content script doesn't need to be long-lived or actually interact with the user. I'd need to add tabs access for it, but other than that I don't foresee any hangups.

@simon-weber simon-weber self-assigned this Jun 18, 2017

@brianzam

This comment has been minimized.

brianzam commented Jun 19, 2017

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 24, 2017

I ran into some small problems:

  • the GA bundle I'm using doesn't use a module system, and it's currently being injected directly by the manifest for content scripts. I'll need to either figure out how to get browserify to support it or just also inject it every time I inject a script.
  • tab.onUpdated seems to fire the loaded event twice for some page refreshes, causing the injected script to post back twice as well. The background script currently doesn't handle this well.

They're nothing too annoying to fix, but it means this will probably take more than just the afternoon.

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 25, 2017

Ok, I fixed both of those and things are largely back to where they were. I just need to handle errors, clean things up, and then add the code to scan tabs on startup. I'll probably ship a version with just those changes before moving on to adding automatic reloads.

@brianzam

This comment has been minimized.

brianzam commented Jun 26, 2017

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 29, 2017

Ok, the switch to script injection is out in 5.3.1. I wouldn't expect much from it yet - it's an entirely lateral change - but it should put us in a good place to automate reloads.

It was a pretty wide reaching change: be on the lookout for anything I broke along the way, haha.

@brianzam

This comment has been minimized.

brianzam commented Jun 29, 2017

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jun 29, 2017

You'll get it automatically, though you will need to re-enable the extension because of the permissions change. You should see a little orange icon up where the page action normally sits that will prompt you to do it.

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jul 12, 2017

Alright, I'm back on this (I just finished moving!). 5.3.2 begins the switch towards dynamically finding a Google Music tab when it's needed, which will eventually be used for finding GM tabs on startup.

This release also collects data on how often people have more than one GM tab open, since in that case I'm planning to just blindly select the first one to talk to. If it's at all common - eg if people are often running one tab hooked up to the extension and one logged into a different account - I may have to be smarter about it.

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jul 13, 2017

5.3.3 now automatically connects to tab at startup. <2% of queries detected multiple tabs, so I went ahead with blindly using the first one.

Everything is in place for automatic reloads now. I'm thinking we'll reload when (the extension tab is not open AND (there are more errors than usual OR we get deauthed). To protect against reload loops, I'll store the last reload time into sync storage and prevent reloads more than once every minute.

@brianzam

This comment has been minimized.

brianzam commented Jul 13, 2017

simon-weber added a commit that referenced this issue Jul 14, 2017

@simon-weber

This comment has been minimized.

Owner

simon-weber commented Jul 15, 2017

Ok, reloads after deauths are out in 5.3.4! I'll track reloads after error spikes separately: it's a bit harder and I want to see the effect of deauths alone.

@simon-weber simon-weber removed their assignment Jul 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment