[Session Restore] TabStateCache is not invalidate or update when persist tab attribute changed

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tabmix.onemen, Assigned: ttaubert)

Tracking

Trunk
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
Attributes that are in TabAttributes set only saved to TabStateCache from SessionStoreInternal._collectBaseTabData.

When one or more of this attributes changed on a tab it doesn't trigger any event that invalidate or update TabStateCache for a given tab and attribute.

So, when SessionStoreInternal._collectTabData return tabData from TabStateCache it can contain outdated tabData.attributes.
Indeed, at the moment, the only TabAttributes-related invalidation I perform is upon a call to persist(). It is not clear to me what I should do, though.

Tim, any suggestion?
Flags: needinfo?(ttaubert)
The only thing we can do here is to use MutationObservers [1] that listen for attribute changes on all tabs and cause delayed saves like all the other state changes.

[1] https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
Flags: needinfo?(ttaubert)
(Reporter)

Comment 3

4 years ago
follow 915173 example.

Don't invalidate the whole tab state when changing TabAttributes.persist
Target Milestone: --- → Firefox 26
Bug 915173 is completely different from this one. We can't just intercept setAttribute() calls like setTabValue(). Please take a look at comment #2.
Target Milestone: Firefox 26 → ---
(Reporter)

Comment 5

4 years ago
With the current code starting form Firefox 26 users will suffer from data lost from persistTabAttribute feature.

very simple fix is to always get up-to-date TabAttributes.get(aTab) by _collectTabData.
with this patch you don't have to observe all tabs for any attribute change

>  _collectTabData: function ssi_collectTabData(aTab) {
>    if (!aTab) {
>      throw new TypeError("Expecting a tab");
>    }
>    let tabData;
>    if ((tabData = TabStateCache.get(aTab))) {
>+     tabData.attributes = TabAttributes.get(aTab);
>      return tabData;
>    }
>    tabData = new TabData(this._collectBaseTabData(aTab));
>    if (this._updateTextAndScrollDataForTab(aTab, tabData)) {
>      TabStateCache.set(aTab, tabData);
>    }
>    return tabData;
>  },
Right, that would be a valid workaround for Fx 26. Not an actual long-term solution, though.
(Reporter)

Comment 7

4 years ago
(In reply to Tim Taubert [:ttaubert] from comment #6)
> Right, that would be a valid workaround for Fx 26. Not an actual long-term
> solution, though.

OK.

(In reply to onemen.one from comment #3)
> follow 915173 example.
> 
> Don't invalidate the whole tab state when changing TabAttributes.persist

I was ment to this
>  persistTabAttribute: function ssi_persistTabAttribute(aName) {
>    if (TabAttributes.persist(aName)) {
>      TabStateCache.clear();
>      this.saveStateDelayed();
>    }
>  },

Is it possible to call
>  TabStateCache.updateField(tab, "attributes", TabAttributes.get(tab));

for all tabs in all windows from persistTabAttribute function
We already call TabStateCache.clear() when the list of persisted attributes changes. That causes us to recollect all attribute values. What we don't notice is when an attribute of a tab changes. That's what this bug is about.
(Reporter)

Comment 9

4 years ago
Any progress ?

this bug already effect us in Firefox 25
Tim, can you decide of a priority for this?
Flags: needinfo?(ttaubert)
I would like to fix this sooner than later but I didn't hear too many real world complaints about this. We currently have bigger fish to fry. I welcome anyone that likes to work on this. What we need is a TabAttrModified listener [1] that updates the cached tab state.

[1] http://mdn.beonex.com/en/Code_snippets/Tabbed_browser.html#Notification_when_a_tab%27s_attributes_change
Flags: needinfo?(ttaubert)
Whiteboard: [good first bug][mentor=ttaubert][lang=js]
Sorry, that's nonsense. The TabAttrModified event is fired for a few attributes only. Definitely not for custom ones. We will in fact need a MutationObserver [1] for every tab out there. I hope that's not too much of a performance impact...

[1] https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver
I take this. And for the clarification, where should I add the |MutationObserver|? I think it could be added in |TabState.jsm| like `tabAttributesObserver: new MutationObserver(callback)` and blah blah to observe the attributes of |TabAttributes| and when they are changed, call the |TabStateCache.set|.
Does it work?
Assignee: nobody → pylaurent1314
Depends on: 960903
Argh, this completely fell off my radar. This will be fixed without us needing to do anything by bug 960903. I have a few patches ready and will attach them soon. Sorry Peiyong for stealing this bug but I really don't want you to work on this just to remove it a week after.
Assignee: pylaurent1314 → ttaubert
Severity: major → normal
Status: NEW → ASSIGNED
Whiteboard: [good first bug][mentor=ttaubert][lang=js]
Fixed by bug 960903.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.