Fix tree structure loss on session restore for moved active tabs - #3846
Conversation
On session restore, waitUntilCompletelyRestored() already calls browser.sessions.getTabValue() to confirm each tab's persistent ID is available, but discards the actual value and returns only a boolean. Later, when Tab.init() runs during rebuildAll(), UniqueId.request() issues the same IPC call again — but updateUniqueId() is fire-and-forget (the returned Promise is never awaited), so loadTreeStructure() proceeds while uniqueIds are still unresolved. attachTabFromRestoredInfo() then fails to find parent tabs via Tab.getByUniqueId(), breaking tree structure. Fix this by having waitUntilCompletelyRestored() return the persistent IDs it already obtained, and passing them to UniqueId via a Map. When UniqueId.request() finds an entry in the Map, it skips the redundant IPC call and resolves synchronously, ensuring all uniqueIds are populated before loadTreeStructure() runs.
… flag for tabs without session tree info When attachTabFromRestoredInfo runs on a recycled active tab during session restore, the tab may not yet have session data (ancestors and children are both empty). Previously the flag was set unconditionally, which blocked the later re-restoration when the actual session data became available via Tab.onRestored. Now we only set the flag when there is actual tree info (ancestors or children) present.
tkng
changed the title
|
|
||
| const windows = await MetricsData.addAsync('init: getting all tabs across windows', promisedWindows); // wait at here for better performance | ||
| if (restoredPersistentIdMap && restoredPersistentIdMap.size > 0) | ||
| UniqueId.setRestoredPersistentIdMap(restoredPersistentIdMap); |
There was a problem hiding this comment.
background.jsがUniqueIdモジュールの内部事情を知りすぎてしまっている感じが気になります。
永続化されたIDに関連する処理という括り方で、waitUntilPersistentIdBecomeAvailable()とwaitUntilCompletelyRestored()の主要部分をUniqueIdモジュールに移してしまった方がよい気がしてきました。
「永続化されたIDが復元されるまで待つ」という見せ方だとこんな感じでしょうか。
const promisedRestored = UniqueId.ensurePersistentIdRestored({
onTabRestored(tab) {
// Read caches from restored tabs while waiting, for better performance.
browser.sessions.getWindowValue(tab.windowId, Constants.kWINDOW_STATE_CACHED_TABS)
.catch(ApiTabs.createErrorSuppressor())
.then(cache => mPreloadedCaches.set(`window-${tab.windowId}`, cache));
browser.sessions.getTabValue(tab.id, Constants.kWINDOW_STATE_CACHED_TABS)
.catch(ApiTabs.createErrorSuppressor())
.then(cache => mPreloadedCaches.set(`tab-${tab.id}`, cache));
},
});必要がなくなったらUniqueId.clearRestoredPersistentIdMap()しないといけない、という事情もできればUniqueIdモジュール内部に閉じさせておきたいです。
UniqueId内でbrowser.tabs.onRemovedを監視して適宜Mapから要素を削除していくようにするのが、モジュール同士の責任を分けるという観点ではベストに思えますが、Mapが長時間メモリー上に存在し続けるデメリットがあり、悩ましいです。
There was a problem hiding this comment.
d399604 で UniqueId.ensurePersistentIdRestored に実装を移しました。同時に、readyToDetectDuplicatedTabとclearRestoredPersistentIdMapを統合して、 completeRestoration という新しい関数を作りました。
| ]); | ||
| ancestors = ancestors || []; | ||
| children = children || []; | ||
| const hasSessionTreeInfo = ancestors.length > 0 || children.length > 0; |
There was a problem hiding this comment.
これだと元々ツリーになっていなかったタブが復元された場合も該当することになると思うのですが、そのことによる副作用がないかが気になります。
影響としては attachTabFromRestoredInfo() が同じタブに何度も実行されるようになる可能性があり、動作が壊れるということはなさそうですが、browser.sessions.getTabValue()の呼び出し回数が増える恐れはあるでしょうか。
ここで特別扱いしないといけないのはクラッシュからの復帰時など手動操作でのセッション復元でrecycleされたタブのときだけなのだとすると、const maybeToBeRecycledTab = tab.active && (ancestors.length > 0 || children.length > 0); if (!maybeToBeRecycledTab) { ... } のように限定してしまってもよい気がしています。
There was a problem hiding this comment.
こちらのブランチをローカルで検証してみていますが、自動セッション復元を無効にして、アプリケーションメニュー→History→Restore Previous Sessionでツリー復元を試みた際にツリー復元に失敗する結果になりました。
background-cache.jsのfixupTabRestoredFromCacheにもtreeStructureAlreadyRestoredFromSessionDataを設定している箇所があり、ここも同様のガードを入れないといけなさそうです。
There was a problem hiding this comment.
c019db0 で tab.active を考慮に入れるようにしてみましたが、いかがでしょうか？
手元ではずっと自動セッション復元は無効にしていて（意図的に無効にしていたわけではなく、設定ができることを知りませんでした）、しかし、「アプリケーションメニュー→History→Restore Previous Session」はメニューが存在しません。Linux版だからですかね？ 起動時のメイン画面に「セッションを復元」ボタンがあるので、いつもはそれを押してセッションを復元しています。
c019db0 を、自動セッション復元が無効の状態でLinux上で使っている限りでは、数回テストした範囲では、ツリー復元に失敗することはありませんでした。（TSTの設定で、キャッシュを有効にしている場合も、無効にしている場合も、うまく行っています。ただ、キャッシュ無効状態だと、セッション復元後、前回開いていたタブではなく、一番下のタブにフォーカスが移動するようになりました。これが以前からこのような挙動だったかは覚えていません。）
ともあれ、手元の環境で同じテストができないので、 c019db0 を一度試していただけると幸いです。
|
ご対応いただきありがとうございます！
という操作を複数回行ってみて、trunkでは何回かに一回ツリーが復元されないのに対し、こちらのPRのブランチでは今のところ安定してツリーが復元されるようになっている様子であることが窺えました。 ただ、操作パターンによってはツリーが復元されずフラットになることが依然ありました。
という経緯で発生している事が分かりました。 diff --git a/webextensions/background/background-cache.js b/webextensions/background/background-cache.js
index fbf05afca..29bbe374c 100644
--- a/webextensions/background/background-cache.js
+++ b/webextensions/background/background-cache.js
@@ -294,7 +294,8 @@ function fixupTabRestoredFromCache(tab, permanentStates, cachedTab, idMap) {
tab.$TST.addState(Constants.kTAB_STATE_PENDING);
}
- tab.$TST.temporaryMetadata.set('treeStructureAlreadyRestoredFromSessionData', true);
+ if (parentTab || childIds.length > 0)
+ tab.$TST.temporaryMetadata.set('treeStructureAlreadyRestoredFromSessionData', true);
}
function fixupTabRestoredFromCachePostProcess(tab) {この事によるデメリットはTab.onRestoredのハンドリング時のコスト増加（session.getTabValueの呼び出しコスト）ですが、ウィンドウ復元やタブの開き直しでツリー構造を復元「できない」ことのデメリットの方が、ツリーになっていなかったタブの復元時に余計な処理が走ることのデメリットより致命的であると考えると、この変更は妥当と言えるように思います。 以上のように考えて、こちらのPRをマージ後に前述の修正をこちらで加えようと思います。 |
…like manual session restoration #3846
…oor#3846) * fix: retain persistent IDs obtained during startup for tree restore On session restore, waitUntilCompletelyRestored() already calls browser.sessions.getTabValue() to confirm each tab's persistent ID is available, but discards the actual value and returns only a boolean. Later, when Tab.init() runs during rebuildAll(), UniqueId.request() issues the same IPC call again — but updateUniqueId() is fire-and-forget (the returned Promise is never awaited), so loadTreeStructure() proceeds while uniqueIds are still unresolved. attachTabFromRestoredInfo() then fails to find parent tabs via Tab.getByUniqueId(), breaking tree structure. Fix this by having waitUntilCompletelyRestored() return the persistent IDs it already obtained, and passing them to UniqueId via a Map. When UniqueId.request() finds an entry in the Map, it skips the redundant IPC call and resolves synchronously, ensuring all uniqueIds are populated before loadTreeStructure() runs. * fix(tree-structure): skip treeStructureAlreadyRestoredFromSessionData flag for tabs without session tree info When attachTabFromRestoredInfo runs on a recycled active tab during session restore, the tab may not yet have session data (ancestors and children are both empty). Previously the flag was set unconditionally, which blocked the later re-restoration when the actual session data became available via Tab.onRestored. Now we only set the flag when there is actual tree info (ancestors or children) present. * refactor(unique-id): move persistent ID restoration logic from background into UniqueId module * fix(tree-structure): narrow restored-flag skip to recycled active tabs only
…like manual session restoration piroor#3846
2 participants
概要
タブを移動した直後にFirefoxをkillして再起動すると、セッション復元時にツリー構造が壊れることがある問題を修正します。問題の発生率は100%ではないもののかなり高く、おそらく、Firefoxのセッション永続化が15秒置きであるのに対し、TSTのキャッシュ永続化が500msの待ち時間で行われるため、2つの永続化情報の矛盾が生じた際にツリー構造が壊れていたものと思われます。
本PRでは2つの根本原因に対処しています:
waitUntilCompletelyRestored()が取得済みの persistent ID を破棄していたため、UniqueId.request()が再度非同期で取得し直す必要がありました。しかしloadTreeStructure()はその完了を待たないため、Tab.getByUniqueId()が親タブを見つけられずツリーが壊れていましたattachTabFromRestoredInfo()がセッションデータ（ancestors/children）が空でも無条件にtreeStructureAlreadyRestoredFromSessionDataフラグを設定していたため、recycled active tab のTab.onRestored経由での再復元がブロックされていました変更内容
起動時に取得した persistent ID を保持する (
background.js,unique-id.js)waitUntilCompletelyRestored()が取得済みの ID をMap<tabId, persistentId>として返すように変更しました（これまでは一旦取得した情報を捨てて、後でもう一度rebuildAll内で非同期的に取得のコードが動いていた）UniqueId.request()はこの Map を先に確認し、エントリがあれば同期的に解決します（冗長なbrowser.sessions.getTabValue()呼び出しを省略します）loadTreeStructure()実行前にすべての uniqueId が解決済みになりますtreeStructureAlreadyRestoredFromSessionDataフラグにガードを追加する (tree-structure.js)ancestors.length > 0 || children.length > 0の場合のみフラグを設定するように変更しましたattachTabFromRestoredInfo()の初回実行時点ではまだセッションデータを持っていない場合があり、空データでフラグを設定すると、後から実際のセッションデータが到着した際の復元がブロックされていました