The default bug view has changed. See this FAQ.

[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
3 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)
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Comment 4

4 years ago
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;
>  },
(Assignee)

Comment 6

4 years ago
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
(Assignee)

Comment 8

4 years ago
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

3 years ago
Any progress ?

this bug already effect us in Firefox 25
Tim, can you decide of a priority for this?
Flags: needinfo?(ttaubert)
(Assignee)

Comment 11

3 years ago
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]
(Assignee)

Comment 12

3 years ago
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
(Assignee)

Updated

3 years ago
Depends on: 960903
(Assignee)

Comment 14

3 years ago
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]
(Assignee)

Comment 15

3 years ago
Fixed by bug 960903.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.