Fixed syncTabsOrder() / Unclickable Tabs (#2199) + Many (Not all) of v3.0.7 Critical Issues (#2238) + Auto Reload Sidebar on Critical Tab Order Failures #2239
Conversation
…en for New Tabs) After Throw Error Corrupts State - Need Auto Tab Resync, Not syncTabsOrder() throw Error" -Now syncTabOrder() actually works -Depends on changes committed in next commits (at least for the auto Reload Sidebar fallback auto-fix portion of this) -Required complete syncTabOrders() rewrite, as it was never actually even attempting to reorder in all (or nearly all) cases, where only would execute it was already in-sync -Now reordering occurs immediately, instead of being deferred (possibly indefinitely/forever) before
…remain out-of-sync -with reloadSidebars() in background.js -Required by previous commit fixing sortTabsOrder() -This should be rarely needed now that sortTabsOrder() actually works, but is critical for auto handling all other cases (or if sortTabsOrder() is ever broken, etc.) especially if opening often or encounter issues with imported or cached results (eg. from previous sessions).
…n background in attempt to reduce frequency of initialization errors and attempt to fix the errors that occur on startup from piroor#2238 -- "Error: Could not establish connection. Receiving end does not exist" -- "Tab opened during init never sets favicon or sometimes title"? -After fixing those can try to remove some/all of those awaits, but may help to pinpoint, avoid or reduce the issues, so suggest keeping until then
--This defaults to true currently, to reduce impact, since may need to test to ensure doesn't prevent session restore caching. --However, I suggest changing useCachedTreeBackgroundExport = false by default, as that avoids "Error: Could not establish connection. Receiving end does not exist"
…kground) as slower, but more complete alternative to reloadSidebars() - if still fail to recover from tab sync or other critical issues, then could auto call this. - Cloud also add context menu item for "Reload Sidebar (Full)" or "Reopen Sidebar" if wanted
… contextTab is undefined.
Note: May need translation still for non-English languages (though added fallback to use English in those cases)
WARN: sendMessage(kCOMMAND_PULL_TABS) in rebuildAll is returning undefined (at least when called or auto-called early on during init, but maybe always). -I fixed the error to handle undefined, but may be underlying issue causing this to happen which may relate to remaining unfixed startup issues described in piroor#2238 -Skipped importTabsFromBackground() when caching is disabled (especially since it has been causing errors) and also if/when configs.useCachedTreeBackgroundExport == false (which I suggest making the default after testing that further).
|
On the branch https://github.com/piroor/treestyletab/tree/PowerAccess-master I'm trying to understand changes and reformat them to match to the design policy of TST's existing codes... |
|
@PowerAccess I've done reformatting of this PR at the branch https://github.com/piroor/treestyletab/tree/PowerAccess-master . Does it still work effectively as you expected? Did I misunderstood something? |
|
Sorry I'm still confusing. This PR looks to having too much things, and they looks should be separated to different changes - especially reducing of the "Error: Could not establish connection. Receiving end does not exist" warning. I think we need to think "waiting until completion to do things stability" and "waiting until completion to reduce warnings" separately. You added many "await" around the initialization process, but I thought that their purpose may be confused. How do you think about that point? |
|
After a research, I've found an information about the warning: the "Could not establish connection" warning appears when |
This change is imported from #2239 and https://github.com/piroor/treestyletab/tree/PowerAccess-master .
|
Now I'm trying to import changes introduced with this PR to the master branch step by step. |
The main idea is inspired from #2239
|
Based on the idea of this PR, I've added codes to rebuild all at both the master process and sidebars, when the list of tabs tracked by the master process is not matched to the native tabs. @PowerAccess, could you confirm that these changes are effective or not to solve "failed to sync" cases on your environment? |
|
Thanks for integrating some of those fixes from my PR, and continuing with that. Tab sync issues still are occurring, though less often. Now, with latest in Git master, when a tab fails to sync and becomes "unclickable" it doesn't cause all the other tabs created after that to also become unclickable. However, tabs still get out-of-sync and remain out-of-sync forever, even 10 minutes later. I explained why in past issue replies in more detail a few of the problems encountered with it, and then encountered even more, with the basic design of it even. That is why I ended up rewriting it, and would suggest you use the version I had written it to. Can you use at least my implementation of syncTabsOrder() as-is vs. applying just parts of it? Also, can you have syncTabsOrder() called automatically every configurable interval (defaulting to 10 seconds ideally, or at least 30 or 60 sec)? Even my rebuildSidebars() implementation ended up with executing so fast and without even any kind of flicker (since it did a lightweight resync more then reload) so that there was no noticeable delay or performance overhead, so I think frequent calls to this should work well. Also, because syncTabsOrder() was almost never called (even when I changed reserveSyncTabsOrder() to execute it immediately, as I suggest you also do), it's even more critical to do that periodically. Plus, I would suggest providing Reload Sidebar like I had implemented, to allow users to recover from more rare issues, to workaround remaining issues (like still have now) and as a workaround for if/when syncing gets broken (as I've noticed more and more tab sync issues occur over time, and can easily get broken again). |
Errors Occurring for Remaining Unclickable Tabs IssueHere are some of the errors I'm encountering with latest from Git master, when I found some tabs unclickable. For example, in this case, when I try to click on "v3.0.7" tab (which is the parent of the active tab in this screenshot below) then it activates the tab I clicked on (at least, which is better than a random tab like before) for like 100ms before auto-switching back to whatever the previous tab was.
Errors in Console with Latest Git Master15:24:39.745 Error: Incorrect argument types for tabs.query. 2 sidebar-tabs.js:408:18 |
Tabs Opened During TST Init IssuesAlso, the Remaining Issue with "Tabs Opened Early During TST Init Always having Loading Animated Favicon" still remains, as detailed in my last reply to Issue #2238. This might apply to tabs opened before TST sidebar fully initializes too, but I tested with TST sidebar always auto opening on Firefox startup so it least occurs for any tabs you open while TST sidebar is shown but still initializing (eg. with progress bar shown). See Steps to Reproduce provided and try using a bookmark in Bookmark Toolbar for "https://trakt.tv/calendars/my/shows" specifically (in case easier to reproduce with that) so that you can click on it to open it very quickly before TST finishes initializing. This is part of why I spent time on the errors like " Please note that my PR did not fix (or at least not completely fix) this issue, though I made some changes which I hoped would help narrow down possible causes of the issue or avoid other issues with tabs opening tabs during TST init, since those tabs always have major issues with them currently, that even Reload Sidebar can't fix. One issue I found which may be relevant to that, when debugging latest from Git master, is: If you enable "Pause on Exceptions" and follow Steps to Reproduce from linked to reply, then you may be able to see that as well: |
|
Thank you for comments. I've not read all yet, but I think we need to build a consensus about the basic strategy of this project.
From now I try to read your comments to understand things you actually did... |
|
Oops, sorry there were some careless mistakes as you commented at #2239 (comment) , and I've fixed them. |
|
After reading the comment #2238 (comment) I've tried the step "Once sidebar is shown, but while still shows loading progress (before fully initialized) I immediately click on a bookmark for:" and I successfully reproduce a failure case: some tabs are not tracked by the master process. Those tabs are opened at the timing between // background/background.js
waitUntilCompletelyRestored().then(() => {
// don't wait at here for better performance
promisedWindows = browser.windows.getAll({
populate: true,
windowTypes: ['normal']
}).catch(ApiTabs.createErrorHandler());
}),
...
// here!
...
ApiTabsListener.startListen();This problem looks unsolvable, because any fundamental solution may make the initialization process blocked and slow. Now I agree that "reload all" operation is required on such cases as a workaround, and I've introduced more fixes to do the auto-reload-all operation triggered by the function |
After a research I've realized that it happens when I open a new tab |
|
After more research I've realized that |
As I commented at #2238 (comment) , I think this kind warning on initialization is a side effect of optimizations.
I thought that it looked impossible to be "resolved" completely due to TST's internal design. After a reconsideration I've realized that those tabs can be tracked without auto-reload. The commit aaf9e11 does that. Now all tab events notified while TST is building tree with tabs fetched via
This should be fixed with the commit 1e54567. ( #2239 (comment) )
I think that the error should be suppressed with some changes I already did. ( #2239 (comment) )
As I commented at #2238 (comment) , I think they are not bugs and just side effects from aggressive optimizations. |
Fixes for #2199 (with syncTabsOrder) + many (but not all) of #2238 (v3.0.7 Critical Issues), including:
A. caching is disabled (configs.useCachedTree = false) OR
B. if configs.useCachedTreeBackgroundExport = false (left = true for now) (not exposed in Options UI)
Remaining Issues from #2238 still not fixed