Closed Bug 655550 Opened 13 years ago Closed 13 years ago

Persisted tab attribute gets lost after restart twice

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 8

People

(Reporter: tabutils+bugzilla, Assigned: tabutils+bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

See the following steps.

Reproducible: Always

Steps to Reproduce:
1. Execute in JS shell:
> gBrowser.selectedTab.setAttribute("visited", true);
> Cc["@mozilla.org/browser/sessionstore;1"]
.getService(Ci.nsISessionStore)
.persistTabAttribute("visited");
2. Restart Firefox and check the "visited" attribute
3. Restart Firefox again and check the "visited" attribute

The "visited" attribute is restored correctly after step 2, but lost after step 3, is it intended?
Your code cannot be executed properly. Please provide a testcase (either attachment or URL) which we can click to test your bug.
Anthony, it's ok. I know what he means. This works fine within the same session, but I haven't tested through restarts yet, but it wouldn't surprise me that it didn't work across multiple restarts. That API predates me, but I always considered it to be a "single session" use. If you wanted it to persist again, you called persistTabAttribute again. That's at least an ok workaround.

If you want more runnable code, you can use what's below in your error console (you'll want to run the commented out code the first time through, but not after - and make sure you open the error console from the same window & have the same tab selected every time)

---

var tab = window.top.opener.gBrowser.selectedTab;
/*
tab.setAttribute("visited", true);
Components.classes["@mozilla.org/browser/sessionstore;1"].getService(Components.interfaces.nsISessionStore).persistTabAttribute("boob");
*/
tab.getAttribute("visited");
Thanks. I know how to workaround it. But if it is used in an add-on and the add-on is disabled temporally for diagnosis purpose, there will be a dataloss.
I have read through the related code, so I asked whether it is intended. Could it be improved to persist saved attributes across sessions?

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2774

2782     for (let name in tabData.attributes)
2783       tab.setAttribute(name, tabData.attributes[name]);

I have an idea. persistTabAttribute could be called here for each saved attribute.
Yea, that sounds like the right sort of behavior. It feels a bit weird to store those attributes at a tab level though. Obviously the value needs to be, but perhaps storing the list at the top level would make more sense. We'd have a default list ("image"), and then anything beyond that would show up in sessionstore.js.

If you're interested in fixing this yourself, I can answer any questions you might have.
I'm just not familiar with how to produce a patch yet. Maybe I'll participate in fixing some bugs in the future, but not now. Thanks.
I suggest the following change:

for (let name in tabData.attributes) {
  tab.setAttribute(name, tabData.attributes[name]);
  if (this.xulAttributes.indexOf(name) == -1)
    this.xulAttributes.push(name);
}

No immediate saveState is needed. We have to examine each stored tabData if we handle this at the top level, which seems unnecessarily too complicated.
(In reply to comment #6)
> I suggest the following change:
> 
> for (let name in tabData.attributes) {
>   tab.setAttribute(name, tabData.attributes[name]);
>   if (this.xulAttributes.indexOf(name) == -1)
>     this.xulAttributes.push(name);
> }
> 
> No immediate saveState is needed.

That's what I was thinking if we went that route.

> We have to examine each stored tabData if
> we handle this at the top level, which seems unnecessarily too complicated.

No, we would just store the attribute names at top level in the session and then when we read a session, those are set once, instead of looping over every tab in every window and looking at tabData.attributes and doing indexOf a lot (we'd still need to loop over attributes). That would also handle the case where an attribute was asked to be persisted, but no tab has that attribute at shutdown.

You're good at reporting bugs when you find them and can figure your way around session restore. Let me know if you decide to participate in the future. I'd be happy to help :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Marking this as a good first bug in case somebody comes along and would like to attribute. This is pretty low priority for me, but would make a pretty good project for people looking to contribute.
Whiteboard: [good first bug][mentor=zpao]
Attached patch patch (obsolete) — Splinter Review
Attachment #536832 - Flags: review?(paul)
(In reply to comment #7)
> No, we would just store the attribute names at top level in the session and
> then when we read a session, those are set once, instead of looping over
> every tab in every window and looking at tabData.attributes and doing
> indexOf a lot (we'd still need to loop over attributes). 

I see. You mean to store the attributes at top level of the session data, but there will be a risk that the attributes are always added but not removed. If we store them only in the tab data, they'll be removed when the tab gets closed. We may use hash object instead of array to avoid indexOf. Is that desirable?

> That would also
> handle the case where an attribute was asked to be persisted, but no tab has
> that attribute at shutdown.

If an extension is asking this, persistTabAttribute will be called explicitly. We won't lose data.

Anyway, setTabValue/getTabValue/deleteTabValue is an alternative way to persist tab attributes.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #536832 - Attachment is obsolete: true
Attachment #536832 - Flags: review?(paul)
Attachment #537108 - Flags: review?(paul)
Attached patch patch v3Splinter Review
Attachment #537108 - Attachment is obsolete: true
Attachment #537108 - Flags: review?(paul)
Attachment #537112 - Flags: review?(paul)
Comment on attachment 537112 [details] [diff] [review]
patch v3

Review of attachment 537112 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the unnecessarily long delay to review this :(

I think you're right that we'd end up with things persisted for too long and I like your approach here. Storing them as keys makes for some good optimizations.
Attachment #537112 - Flags: review?(paul) → review+
http://hg.mozilla.org/integration/fx-team/rev/7dfbf51f63c5
Assignee: nobody → ithinc
Whiteboard: [good first bug][mentor=zpao] → [fixed-in-fx-team]
http://hg.mozilla.org/mozilla-central/rev/7dfbf51f63c5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Blocks: 675493
You need to log in before you can comment on or make changes to this bug.