737851 - Generate form state key at page load instead of page unload

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Given that elements created after page load will unlikely exist at page reload, we could probably just generate the form state key at page load so we would make sure to be able to find the same elements on reload in most cases. For the edge cases, we could also generate a key at page unload if the element doesn't have a key yet.

Boris, that's your idea originally, do you confirm it's worth trying it?

Comment 1

[Mounir Lamouri (:mounir)]

I'm adding a few bugs in blocking list. There might be others though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Boris, that's your idea originally, do you confirm it's worth trying it?

Well, _I_ clearly think it's a good idea... ;)

Bug 660549 has the relevant discussion.  Bug 660549 comment 5 talks about what WebKit does here.

Comment 3

[Mounir Lamouri (:mounir)]

Louis, I think the patch you wrote in bug 660549 would be more appropriate in this bug.

Comment 4

[Louis Gesbert]

Attachment: This is a new version of the patch submitted to bug 660549

Does exactly what Boris suggested: generates the state-key at loading time, stores it, and reuses it when saving. Following Mounir's recommendations, it's now done in a new class instead of bloating nsHTMLGenericFormElement.

There's a few things I am not sure about: first, if we should silently generate the key at save-time if it failed at load time, as Boris suggested above. In that case, it doesn't need to appear in the interface, it's really a lazy variable (in this case I could remove explicit calls to GenerateStateKey from child classes; I didn't want to do too much magic).

Also, I'm wondering wether we should generate the state key even if mInhibitRestoration is set: is it permanent, or can its state change over time ? (and maybe mInhibitStateRestoration could even be moved to the new class and tested there).

Also, I must admit I am still a little puzzled by these NS_FORWARD_NSIDOMNODE and such. For example, nsHTMLInputElement forwards nsHTMLGenericFormElement, which was its parent. Now that there is nsHTMLGenericFormElementWithState between the two, should the forwarding be updated ? Any explanation of the consequences would be welcome :)

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 608397 [details] [diff] [review]
This is a new version of the patch submitted to bug 660549

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

I wonder if we should generate the key when the document fires the load event to take into account elements being moved/added during the document load? Also, if we generate a key at unload when not present, can't we end up having two elements with the same key if they have moved during the document life? (I don't know the details of key generations so I don't know if we have a mechanism against this)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I wonder if we should generate the key when the document fires the load event 

Basically, we should generate the key when we would be restoring state.  That's the point at which we would use the key, so that's the point when it should be generated.

Do we wait until after onload to restore state?  I don't think we do.

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> Do we wait until after onload to restore state?  I don't think we do.

Indeed.

Comment 8

[Louis Gesbert]

I've written some tests for nsHTMLGenericFormElementWithState, will submit a patch as soon as they go smoothly in the reftests (I guess I'll have to go through an iframe to get the page to reload correctly).

Two suspicious cases:
- dynamically generated inputs (document.createElement) aren't restored
- cloned inputs have the same key, so get restored to the same state. Maybe we should re-generate the key when cloning, that would probably solve the most common case but cause some more weirdness in others.

Anyway, in both cases, webkit exhibits the same behaviour (except that you need to forward/back, reload clears the form).

Comment 9

[Louis Gesbert]

Attachment: As promised, some tests

They pass with the patch, but also show the limit cases mentioned above (field not restored / restored twice when dynamically generating / cloning input fields); the ref can be updated when we make a decision on those.

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 609735 [details] [diff] [review]
As promised, some tests

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

I think it would be more appropriate to do a Mochitest. Given that it is related to forms, you should put it in content/html/content/test/forms/ there is already a test for save/restore there already (test_save_restore_radio_groups.html) so it shouldn't be too hard to adapt your test.

Comment 11

[Louis Gesbert]

Attachment: Rewritten the test as Mochitest

Comment 12

[Louis Gesbert]

Attachment: Rewritten the test as Mochitest

Oops, forgot to remove a debug line (local script src)

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 611160 [details] [diff] [review]
Rewritten the test as Mochitest

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

I prefer tests that clearly differentiate different parts because it's easier to have an overview and understand what is actually tested. However, it's not a big deal.

A note regarding the coding style: you should add more empty lines in your code, it would be nicer to read ;)

I did not r+ the test because there is one change I would like to see (the iframe). Otherwise, it looks good.

::: content/html/content/test/forms/test_restore_form_elements.html
@@ +7,5 @@
> +  <meta charset="utf-8">
> +  <title>Test for Bug 737851</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +<body onload="mainOnLoad();">

Could you use addLoadEvent() instead?

@@ +46,5 @@
> +<script type="application/javascript">
> +
> +/** Test for Bug 737851 **/
> +
> +/* We reload the page once to check persistence of elements */

Could you use an iframe instead of reloading the entire test?

@@ +128,5 @@
> +      SimpleTest.is(document.getElementById(id).value, fieldValues[id],
> +                    "Field "+id+" should be restored after reload");
> +    } else {
> +      SimpleTest.todo_is(document.getElementById(id).value, fieldValues[id],
> +                         "Field "+id+" isn't properly restored after reload");

Could you explain why do you need that? I mean, are there some failures?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please create new files for your new class; nsGenericElement.* is too big already. (Also: move it into the mozilla::dom namespace.)

Comment 15

[Louis Gesbert]

Attachment: Mochitests for this bug (and the one it blocks)

Cleaned up according to Mounir's review. Thanks !

Comment 16

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here; this is at the top of my list for Monday.

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 612871 [details] [diff] [review]
Mochitests for this bug (and the one it blocks)

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

I'm afraid I will have to ask you for another patch.

::: content/html/content/test/forms/inner_restore_form_elements.html
@@ +62,5 @@
> +      inputE2.setAttribute("id","e2");
> +      testdiv.appendChild(inputE2);
> +    }
> +
> +    addLoadEvent(shuffle);

That could have been done in the outer frame.

::: content/html/content/test/forms/test_restore_form_elements.html
@@ +21,5 @@
> +
> +<div id="content"><!-- style="display: none">-->
> +
> +  <iframe id="frame" width="800px" height="600px"
> +          src="inner_restore_form_elements.html">

I would highly prefer a data url instead of a new file. It's way harder to read a test in multiple files.

@@ +38,5 @@
> +
> +var frameElement = document.getElementById("frame");
> +var frame = frameElement.contentWindow;
> +
> +frameElement.addEventListener("load", frameOnLoad1);

You could do:
addLoadEvent(function() {
  frameElement.addEventListener("load", frameOnLoad2);
  fill();
  frame.location.assign(frame.location.href + '#');
  frame.location.reload();
});

That would be safer IMO.

@@ +84,5 @@
> +/* Simulate user input by entering the values */
> +
> +function fill() {
> +  for (id in fieldValues) {
> +    frame.document.getElementById(id).value += fieldValues[id];

+= or =? Why do you need the + here?

@@ +94,5 @@
> +/* Check that all the fields are as they have been entered */
> +
> +function checkAllFields() {
> +  SimpleTest.isnot(frame.document.URL.indexOf("#"), -1,
> +                   "Document should be reloaded once (self-check)");

I would prefer something like:
if (frame.document.URL.indexOf("#") == -1) {
  ok(false, "Something went wrong: the document wasn't reloaded!");
}

So you don't pollute the tests results if everything was okay.

@@ +98,5 @@
> +                   "Document should be reloaded once (self-check)");
> +
> +  for (id in fieldValues) {
> +    var fieldValue = frame.document.getElementById(id).value;
> +    if (typeof(changedFields[id]) === 'undefined') {

if (changedFields[id] === undefined) {
would be equivalent AFAIK (maybe not with some strict)

@@ +103,5 @@
> +      SimpleTest.is(fieldValue, fieldValues[id],
> +                    "Field "+id+" should be restored after reload");
> +    } else {
> +      SimpleTest.is(fieldValue, changedFields[id],
> +                    "Field "+id+" normally gets a different value after reload");

Please, don't use |SimpleTest.| for is(), ok(), isnot(), etc.

Comment 18

[Louis Gesbert]

Attachment: Mochitest, cleaned up according to review

Comment 19

[Mounir Lamouri (:mounir)]

Comment on attachment 613943 [details] [diff] [review]
Mochitest, cleaned up according to review

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

r=me

Though, I would prefer to have the file named "test_save_restore_form_controls.html" for consistency with "test_save_restore_radio_groups.html".

Comment 20

[Louis Gesbert]

Boris: ping ?

I am particularly interested in whether the NS_FORWARD_NSIDOMNODE should be changed ; and we should agree on the results (I made the edge cases visible in the test).
Thanks!

Comment 21

[Boris Zbarsky [:bzbarsky]]

Forwarding to the direct superclass is the right thing to do.

I'm still thinking through the rest of the patch; trying to figure out whether the key generation algorithm we're using here is really what we want.  It might be that it is, or at least that changing it could be a separate bug, but as long as we're here....

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 608397 [details] [diff] [review]
This is a new version of the patch submitted to bug 660549

I'm sorry it took me so long to get to this.  :(

I think overall this is fine, but we should definitely file a followup bug on reevaluating our state key generation to see whether we can do simpler keys in the new setup.  I'm somewhat worried that generating state keys the way we do right now will slow down pageload.  Note that before this patch we have an optimization to not do key generation on pageload if there's no layout history (in other words, for normal page loads). That's the aRead boolean in GetLayoutHistoryAndKey.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp	Thu Mar 22 18:47:33 2012 +0100

>+nsGenericHTMLFormElementWithState::GenerateStateKey()
>+  // Get the pres shell

There's no presshell here.  Just nix this comment, please.

>+  nsCOMPtr<nsIDocument> doc = GetDocument();

I think this can just be an nsIDocument*

>+  nsCAutoString stateKey;
>+  nsresult rv = nsContentUtils::GenerateStateKey(this, doc,
>+                                                 nsIStatefulFrame::eNoID,
>+                                                 stateKey);
>+  if (NS_FAILED(rv)) {
>+    // mStateKey unchanged (void)
>+    return rv;
>+  }

Would it make sense to generate into mStateKey directly and just SetIsVoid() in the rare failure case?

>+++ b/content/html/content/src/nsGenericHTMLElement.h	Thu Mar 22 18:47:33 2012 +0100
>+  /* Used to store the key to that element in the session. Is void until
>+     InitStateKey has been used */

s/InitStateKey/GenerateStateKey/

>+++ b/content/html/content/src/nsHTMLButtonElement.cpp	Thu Mar 22 18:47:33 2012 +0100
> nsHTMLButtonElement::DoneCreatingElement()
>+  nsresult rv = GenerateStateKey();
>+
>+  if (!mInhibitStateRestoration && NS_SUCCEEDED(rv)) {
>+    RestoreFormControlState();

mInhibitStateRestoration should probably inhibit saving the state too.  So just don't generate a state key when mInhibitStateRestoration, please?

Similar for other elements.

We should probably SetVoid() the state key if we're adopted into a different document.

r- for the above nits, and because the NS_FORWARD bits need fixing, but I promise a much much faster review on the next iteration.

And please let me know if you can't work on this any more at this point.  Again, I'm really sorry this took so long.

Comment 23

[Louis Gesbert]

Sorry for the lag ; just a note to say I'm still on this, only I've also been moving, etc., and am still without a proper Internet connection. I'll get back to it very soon!

Comment 24

[Mina Almasry]

Louis, are you still working on this? Can I take this over?

And if so, I think the work to be done are the nits and NS_FORWARD, correct? Boris I think you never weighed in on what to do with the NS_FORWARD bits. Where should I start with that?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Boris I think you never weighed in on what to do with the NS_FORWARD bits.

See comment 21.

Comment 26

[Mina Almasry]

Attachment: Patch

So far I've done the things in Boris's last review. There are a few things I don't understand:

> We should probably SetVoid() the state key if we're adopted into a different document.

Sorry where does the adopting happen in the code?

Also, I've read comment #21, but I still don't understand what's needed. Can you please elaborate on what the problem with the NS_FORWARD bits is and where do I start with fixing it?

Aside from these things, and the tests is there anything else to be done? Can you point me to where I start with these too?

I have attached the patch I have so far, but I'm not asking for a review since the changes from the last one are minimal... but feel free to review of course.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Sorry where does the adopting happen in the code?

nsNodeUtils::CloneAndAdopt.  The adoption case is where the NodeInfoChanged() call happens and then the various event listener recompilation, etc.

> Can you please elaborate on what the problem with the NS_FORWARD bits is

Looks like we always just forward nsIDOMNode to nsINode, so there is no longer a problem for that forwarding.

But what needs to happen is that various things that reference the parent class need to reference the new parent class.  So any time nsGenericHTMLFormElement is referenced in HTMLButtonElement.cpp, for example should probably be referencing the new parent class instead.

> Aside from these things, and the tests is there anything else to be done? 

Probably nothing; I'll need to re-review the patch carefully at some point, though.

Comment 28

[Mina Almasry]

Attachment: patch with test

Alright it took a bit of tweaking and testing but here is a patch that passes all the tests that mounir reviewed+

It's a long patch, but most of it is redirecting to the new superclass.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Mina, it'll probably take me a few days to get to this.

Comment 30

[Mina Almasry]

No problem :-)

Comment 31

[Mina Almasry]

Just a friendly reminder this waits for a review. Thanks!

Comment 32

[Boris Zbarsky [:bzbarsky]]

Yes, I do remember that.  I just haven't had a chance to sit down to do a longish review... :(

Comment 33

[Mina Almasry]

No problem. Don't be sad :-)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 776077 [details] [diff] [review]
patch with test

>+++ b/content/html/content/src/HTMLButtonElement.cpp
> HTMLButtonElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
>+  nsresult rv = nsGenericHTMLFormElementWithState::BindToTree(aDocument, aParent,
>                                                      aBindingParent,
>                                                      aCompileEventHandlers);

Please fix the indent.  Probably by putting a linebreak after the '=' and then lining up the arguments correctly.

> HTMLButtonElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>+  return nsGenericHTMLFormElementWithState::BeforeSetAttr(aNameSpaceID, aName,
>                                                  aValue, aNotify);

Fix indent.

>@@ -489,29 +492,29 @@ HTMLButtonElement::AfterSetAttr(int32_t 
>+  return nsGenericHTMLFormElementWithState::AfterSetAttr(aNameSpaceID, aName,
>                                                 aValue, aNotify);

Ditto.

>+++ b/content/html/content/src/HTMLInputElement.cpp
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(HTMLInputElement,
>-                                                  nsGenericHTMLFormElement)
>+                                                  nsGenericHTMLFormElementWithState)

The indent here was broken before too, but fix it while you're changing this.

> HTMLInputElement::DoneCreatingElement()
>+  nsresult rv = GenerateStateKey();

This is wasted work if mInhibitRestoration.  Please don't call this if mInhibitRestoration is true.

So perhaps something like:

  bool restoreCheckedState =
    !mInhibitRestoration && NS_SUCCEEDED(GenerateStateKey()) && RestoreFormControlState();

and take out the standalone GenerateStateKey call or some such.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLFormElementWithState::GetLayoutHistory(bool aRead)
>+  // Get the pres shell

No presshell anywhere here.  Please take that comment out.

>+++ b/content/html/content/src/nsGenericHTMLElement.h
>+class nsGenericHTMLFormElementWithState: public nsGenericHTMLFormElement

Space before ':', please.

Please add newlines between these lines:

>+  nsresult GetPrimaryPresState(nsPresState** aPresState);
>+    GetLayoutHistory(bool aRead);

and the following comments.

If you can figure out a way to test the "clear state key on adopt" case, that would be awesome, but I'm fine either way on that.

r=me with the above nits, and very sorry for the terrible lag...

Comment 35

[Mina Almasry]

Attachment: Patch with changes

Changes made, but I also found that this patch is out of date for the current m-c, so I had to make some more changes. The only ones with any significance are in nsGenericFormElementWithState::GetPrimaryPresState, but the function still does the same thing.

Not sure if I should ask for another review. I'm assuming no, so here is a try push:

https://tbpl.mozilla.org/?tree=Try&rev=886a35063b60

Comment 36

[Boris Zbarsky [:bzbarsky]]

I'd actually appreciate an interdiff between those last two patches...

Comment 37

[Mina Almasry]

Attachment: Interdiff

Interdiff!

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 784499 [details] [diff] [review]
Interdiff

Thanks for the interdiff!

r=me

Comment 39

[Mina Almasry]

My pleasure!

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4173c694fb43

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4173c694fb43

Comment 42

[Mounir Lamouri (:mounir)]

Mina, I think it would be good to go trough the bugs marked as blocked by this one and see if this patch solved them and if not what should be done. Do you think you can find some cycles to do that?

Comment 43

[Mina Almasry]

> Do you think you can find some cycles to do that?

Will take a look :-)

Comment 44

[Boris Zbarsky [:bzbarsky]]

Marking qawanted for the issue from comment 42.

Comment 45

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old

Comment 47

[u279076]

I'm dropping the qawanted request on this bug since it's well over a year old. Please re-add the qawanted keyword if the request is still valid.