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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[tabmix.onemen]

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.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 3

[tabmix.onemen]

follow 915173 example.

Don't invalidate the whole tab state when changing TabAttributes.persist

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Bug 915173 is completely different from this one. We can't just intercept setAttribute() calls like setTabValue(). Please take a look at comment #2.

Comment 5

[tabmix.onemen]

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;
>  },

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Right, that would be a valid workaround for Fx 26. Not an actual long-term solution, though.

Comment 7

[tabmix.onemen]

(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

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 9

[tabmix.onemen]

Any progress ?

this bug already effect us in Firefox 25

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Tim, can you decide of a priority for this?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 13

[Peiyong Lin[:lpy](UTC+8)]

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?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Fixed by bug 960903.