Last Comment Bug 662743 - Session restore should do more than restore a <select>'s selectedIndex
: Session restore should do more than restore a <select>'s selectedIndex
Status: RESOLVED FIXED
[good first bug][mentor=zpao]
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Bellindira Castillo [:bellindira]
:
Mentors:
Depends on: 697903
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-07 23:45 PDT by Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
Modified: 2013-11-12 00:57 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (4.24 KB, patch)
2011-06-17 20:41 PDT, Teddy Ni
paul: feedback+
Details | Diff | Review
Rebase patch + minor fix and test case (9.58 KB, patch)
2012-05-03 08:48 PDT, Bellindira Castillo [:bellindira]
no flags Details | Diff | Review
Rebase patch + minor fix and test case (9.48 KB, patch)
2012-05-03 12:08 PDT, Bellindira Castillo [:bellindira]
paul: review-
Details | Diff | Review
Patch v2 and test case. (11.94 KB, patch)
2012-05-09 15:52 PDT, Bellindira Castillo [:bellindira]
paul: review+
Details | Diff | Review
Patch v3 and test. (10.42 KB, patch)
2012-05-17 22:33 PDT, Bellindira Castillo [:bellindira]
paul: review+
Details | Diff | Review
Patch for checkin (10.42 KB, patch)
2012-05-22 01:57 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review

Description Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2011-06-07 23:45:48 PDT
When saving form data, session restore will save a <select> element's selectedIndex.  If, when the page is being reloaded as part of a restore, the cached version of the page isn't used and instead it is loaded again from the network, and the page happens to have changed such that <select> has more/different values, then the restored selectedIndex is not always going to be the right value to use.

This comes up with Bugzilla if a new Product/Component is added between the session being saved and being restored, then you can end up accidentally (if you don't notice the incorrect values upon restore) changing fields you don't mean to.

Storing the selected <option>'s value rather than the <select>'s selectedIndex might work better.
Comment 1 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-06-08 08:00:55 PDT
I'm curious. If you've actively selected something in a select box I assume that's the choice you wanted to make before Firefox crashed or restarted. I assume any new selection which are not appearing would be inconsequential to that choice. Am I incorrect in that assumption?
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-08 10:43:16 PDT
(In reply to comment #1)
> I'm curious. If you've actively selected something in a select box I assume
> that's the choice you wanted to make before Firefox crashed or restarted. I
> assume any new selection which are not appearing would be inconsequential to
> that choice. Am I incorrect in that assumption?

I think you might be misunderstanding. Let's try this: I have form with <select> that have 3 options (* is selected

option foo
* option bar
option baz

Then you crash or quit. Firefox has saved that you selected option at index 1. Between crashing and restoring the server makes changes and adds <option qux> before <option bar>. When you restore your session, we'll reselect option at index 1 (regardless of value). So (if you don't get the cached page) the page looks like

option foo
* option qux
option bar
option baz

The proposal is to use selectedIndex in conjunction with the <option>'s value to cross check. We'd see that index 1 has a value different than what we saved, so we'd then use the value we saved to try to select a different <option>
Comment 3 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-06-08 10:49:16 PDT
Ok, that makes sense to me. Thanks for providing that example. I'm not sure what the likelihood of this scenario occurring, however. Anecdotally it seems like this could be pretty edge-casey. Maybe a "good first bug"?
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-08 10:58:09 PDT
(In reply to comment #3)
> Ok, that makes sense to me. Thanks for providing that example. I'm not sure
> what the likelihood of this scenario occurring, however. Anecdotally it
> seems like this could be pretty edge-casey. Maybe a "good first bug"?

Yea, it doesn't happen often. A bunch of people were annoyed by it recently when a component was added to bugzilla, but that was the first I'd heard of it.

I'll mark it [good first bug] in case anybody wants to grab it. I've been looking at some of this code recently so it's fresh, but low priority.
Comment 5 Teddy Ni 2011-06-17 20:41:19 PDT
Created attachment 540200 [details] [diff] [review]
patch v1

save and restore selection values instead of index, needs testing
Comment 6 Teddy Ni 2011-06-17 20:43:49 PDT
I still need to do some testing on this patch (maybe create something for the test/browser directory, although I admit I don't know the format of those. Thought I'd post it up though as my first, small foray into the code.
Comment 7 Josh Matthews [:jdm] 2011-06-22 09:04:01 PDT
Comment on attachment 540200 [details] [diff] [review]
patch v1

Paul, I think some sort of feedback here for Theodore's patch would be appreciated.
Comment 8 Frank Yan (:fryn) 2011-06-26 23:37:09 PDT
Marking as blocking bug 539228.
If we expand this bug's scope, we could dupe that to this.
Comment 9 Mounir Lamouri (:mounir) 2011-06-27 00:46:22 PDT
I do not think bug 539228 is directly related to this bug.
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-27 11:35:45 PDT
(In reply to comment #9)
> I do not think bug 539228 is directly related to this bug.

You are correct. Session restore is distinct from back/forward/reload form restore. There's a bug somewhere on making session restore use whatever is being stored in the other thing.
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-27 16:03:38 PDT
Comment on attachment 540200 [details] [diff] [review]
patch v1

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

Hey Teddy, Thanks for taking a stab at this. I apologize for the delay getting back to you.

I think this is looking good, but it will need a test. It might be a little tricky since the test page wouldn't be server generated, but I bet you could do it by changing the url to something like test.html?loaded then close it. have a check in the page that does a document.write to add an item if url ~= test.html?loaded
Comment 12 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-30 13:20:29 PDT
Teddy, have you had a chance to start working on a test?
Comment 13 Frank Yan (:fryn) 2012-03-14 17:09:24 PDT
Paul, is writing a test the only thing remaining for this?
Comment 14 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-19 13:16:59 PDT
(In reply to Frank Yan (:fryn) from comment #13)
> Paul, is writing a test the only thing remaining for this?

I never gave it a proper review, but I don't remember anything else being a major issue.
Comment 15 Bellindira Castillo [:bellindira] 2012-04-27 13:23:24 PDT
I'll do the test case and rebase if necessary.
Comment 16 Bellindira Castillo [:bellindira] 2012-05-03 08:48:40 PDT
Created attachment 620719 [details] [diff] [review]
Rebase patch + minor fix and test case
Comment 17 Bellindira Castillo [:bellindira] 2012-05-03 12:08:04 PDT
Created attachment 620804 [details] [diff] [review]
Rebase patch + minor fix and test case

Fixed patch (don't dispatch a change event when select has not changed).
Comment 18 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-04 12:29:53 PDT
Comment on attachment 620804 [details] [diff] [review]
Rebase patch + minor fix and test case

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

Thanks for picking this up Bellindira! I know some of these comments really apply to the patch you picked up but it's yours now :)

About the test: it only checks restoring, but not collection. Can you check that as well? Additionally, you don't check that the old format can still be used (and as I mentioned, it looks like you're breaking that compatability). r- mostly for these 2 major points.

You may also want to ccoordinate with Andres since he (or you depending on who lands first) will need to update your respective patches with bug 697903 and bug 742051.

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2348,5 @@
>        else if (!node.multiple) {
>          // <select>s without the multiple attribute are hard to determine the
>          // default value, so assume we don't have the default.
>          hasDefaultValue = false;
> +        value = { selectedIndex: node.selectedIndex, savedValue: node.value };

nit: let's just call savedValue value (to match the attributes)

@@ +2356,5 @@
>          // default value since each <option> has a defaultSelected
>          let options = Array.map(node.options, function(aOpt, aIx) {
>            let oSelected = aOpt.selected;
>            hasDefaultValue = hasDefaultValue && (oSelected == aOpt.defaultSelected);
> +          return oSelected ? aOpt.value : -1;

This may lead to extraneous items being selected... it wouldn't be great form by the HTML author, but there could be multiple <option>s with the same value at different indices. If only 1 was selected before, we'll select all of them this on restore now.

That's probably fine though

@@ +3516,5 @@
>  
>            node.checked = value;
>            eventType = "change";
>          }
> +        else if (value && value.selectedIndex >= 0 && value.savedValue) {

savedValue could be an empty string and so we would never enter this block. Let's change that to "savedValue" in value.

Also, by getting rid of the pure number check, you're going to break the reading of old files, which we can't do without some transitionary period.

@@ +3532,2 @@
>              eventType = "change";
>            } catch (ex) { /* throws for invalid indices */ }

I don't think we can throw anymore since we're staying within the bounds of node.options.length. So we can probably remove the try/catch or at least update the catch comment.
Comment 19 Bellindira Castillo [:bellindira] 2012-05-04 14:10:56 PDT
Thanks Paul, I'll check this with Andres and make the corrections.
Comment 20 Bellindira Castillo [:bellindira] 2012-05-09 15:52:11 PDT
Created attachment 622558 [details] [diff] [review]
Patch v2 and test case.

Test case checks restoring and collection. Also, it checks old format now.
Updated patch with bug 697903 and bug 742051.
Changed savedValue to value.
Kept backwards compatibility (select - old format).
Removed the try/catch.
Comment 21 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-05-17 11:25:22 PDT
Comment on attachment 622558 [details] [diff] [review]
Patch v2 and test case.

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

Just a couple nits and it's good to go. Thanks Bellindira! Let's get this landed ASAP and then I'll write a blog post about the various format changes that have landed.

::: browser/components/sessionstore/src/DocumentUtils.jsm
@@ +188,5 @@
>          aNode.selectedIndex = aValue;
>          eventType = "change";
> +      } 
> +    } else if (aValue && aValue.selectedIndex >= 0 && aValue.value) {
> +      //handle select new format

Nit: space following //

@@ +190,5 @@
> +      } 
> +    } else if (aValue && aValue.selectedIndex >= 0 && aValue.value) {
> +      //handle select new format
> +      if (aNode.options[aNode.selectedIndex].value == aValue.value) 
> +        return; // don't dispatch a change event for no change

Nit: brace the return and put the comment above
> // comment
> if (foo) {
>   return;
> }

::: browser/components/sessionstore/test/browser_662743.js
@@ +29,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK *****/

Nit: most of our tests are public domain nowadays (see https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_739805.js). Since you aren't an employee of MoCo, you don't have to do that but in that case Mozilla Foundation isn't the initial developer. If you keep MPL, it's 2012 now, not 2011!
Comment 22 Bellindira Castillo [:bellindira] 2012-05-17 22:33:52 PDT
Created attachment 625010 [details] [diff] [review]
Patch v3 and test.

Thanks Paul! Here is a new patch with the fixes of the nits.
Comment 23 Raymond Lee [:raymondlee] 2012-05-21 18:54:15 PDT
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=6c72578254b3
Comment 24 Raymond Lee [:raymondlee] 2012-05-22 01:57:02 PDT
Created attachment 625932 [details] [diff] [review]
Patch for checkin

Passed Try
Comment 25 Tim Taubert [:ttaubert] 2012-05-22 01:58:10 PDT
Will push this in a sec.
Comment 26 Tim Taubert [:ttaubert] 2012-05-22 02:22:59 PDT
https://hg.mozilla.org/integration/fx-team/rev/b1dc93af542d
Comment 27 Tim Taubert [:ttaubert] 2012-05-22 06:26:19 PDT
https://hg.mozilla.org/mozilla-central/rev/b1dc93af542d
Comment 28 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2012-05-22 17:02:23 PDT
Thanks Bellindira and Teddy!
Comment 29 Eric Shepherd [:sheppy] 2012-05-24 06:15:16 PDT
Need to document the changes to the JSON format; see http://zpao.com/posts/session-restore-changes-in-firefox-15/ for details.

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