Last Comment Bug 655550 - Persisted tab attribute gets lost after restart twice
: Persisted tab attribute gets lost after restart twice
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: ithinc
:
Mentors:
Depends on:
Blocks: 675493
  Show dependency treegraph
 
Reported: 2011-05-07 21:10 PDT by ithinc
Modified: 2011-07-31 04:56 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.34 KB, patch)
2011-06-02 00:35 PDT, ithinc
no flags Details | Diff | Review
patch v2 (4.22 KB, patch)
2011-06-03 01:37 PDT, ithinc
no flags Details | Diff | Review
patch v3 (4.51 KB, patch)
2011-06-03 02:00 PDT, ithinc
paul: review+
Details | Diff | Review

Description ithinc 2011-05-07 21:10:42 PDT
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?
Comment 1 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-05-10 16:12:10 PDT
Your code cannot be executed properly. Please provide a testcase (either attachment or URL) which we can click to test your bug.
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-10 16:31:24 PDT
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");
Comment 3 ithinc 2011-05-11 01:34:27 PDT
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.
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-12 13:41:31 PDT
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.
Comment 5 ithinc 2011-05-13 06:48:55 PDT
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.
Comment 6 ithinc 2011-05-13 07:03:19 PDT
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.
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-13 14:55:09 PDT
(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 :)
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-16 13:56:07 PDT
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.
Comment 9 ithinc 2011-06-02 00:35:15 PDT
Created attachment 536832 [details] [diff] [review]
patch
Comment 10 ithinc 2011-06-02 01:22:18 PDT
(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.
Comment 11 ithinc 2011-06-03 01:37:53 PDT
Created attachment 537108 [details] [diff] [review]
patch v2
Comment 12 ithinc 2011-06-03 02:00:23 PDT
Created attachment 537112 [details] [diff] [review]
patch v3
Comment 13 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-26 14:05:54 PDT
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.
Comment 14 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-28 14:20:15 PDT
http://hg.mozilla.org/integration/fx-team/rev/7dfbf51f63c5
Comment 15 Tim Taubert [:ttaubert] 2011-07-30 21:43:12 PDT
http://hg.mozilla.org/mozilla-central/rev/7dfbf51f63c5

Note You need to log in before you can comment on or make changes to this bug.