497861 - [HTML5][Patch] Wrong form state preservation on reparse

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Henri Sivonen (:hsivonen)]

http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug390735.html
and 
http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug345339.html
fail.

It seems that the HTML5 parser doesn't do all the right tricks with form controls.

Comment 1

[Henri Sivonen (:hsivonen)]

nsGenericHTMLElement::RestoreFormControlState runs then it should. However, the line
rv = history->GetState(key, &state);
always writes 0x0 into state.

I guess the problem is state saving--not restoring. (Unless the keys are computed wrong upon restoration.)

Comment 2

[Henri Sivonen (:hsivonen)]

Somehow the document manages to get into a state where both mScriptGlobalObject and mLayoutHistoryState on nsDocument are null. As far as I can tell, that's a bad state even though I don't see it asserted.

Comment 3

[Henri Sivonen (:hsivonen)]

In the HTML5 case, the inputs are unbound from the tree by the cycle collector. Presumably after nsDocument::Destroy() has caused both mScriptGlobalObject
and mLayoutHistoryState to become null.

Comment 4

[Henri Sivonen (:hsivonen)]

With the old parser, the inputs are unbound from tree synchronously in response to nsDocument::Destroy().

I suspect there's a script blocker or something that prevents nsDocument::Destroy() from destroying all the way synchronously in the case of the HTML5 parser.

Maybe nsIParser::Terminate() needs to terminate the doc update batch created by nsHtml5TreeOpExecutor::Flush() if ::Flush() is on the call stack.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: WIP

Comment 6

[Henri Sivonen (:hsivonen)]

Now I'm really confused. With the patch, content/base/test/test_bug345339.html fails 2 and succeeds 4. Why would 2 fields out of 6 in the same test fail? Why don't they all succeed or fail together?

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Better WIP

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Stacks to type=file SaveState with the old parser

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Stacks to type=file SaveState with the HTML5 parser

Comment 10

[Boris Zbarsky [:bzbarsky]]

Henri, what does GenerateStateKey return for the file input in the old case?  In the new case?

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to comment #10)
> Henri, what does GenerateStateKey return for the file input in the old case? 
> In the new case?

type=hidden
Old parser, all calls: 0>0>8>d>4>
HTML5 first and third call: 0>0>8>d>>o>6>2>1
HTML5 second call: 0>0>8>d>4>

type=file
Old parser, all calls: 0>0>7>d>5>
HTML5 first and third call: 0>0>7>d>>o>8>2>1
HTML5 second call: 0>0>7>d>5>

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  So the key difference there is that at those first and third GenerateStateKey calls the control doesn't seem to be in the list returned by htmlDocument->GetFormControls().  As a result, the htmlFormControls->IndexOf(aContent, PR_TRUE) call (which, note, flushes the sink notifications in the old parser; this is a known suck issue) returns -1, which means that after the ">d" we just append ">" for the name instead of the ">5>" for the control index plus name.  Then we fall into the !generatedUniqueKey block and append the ">o..." stuff.

Which controls are failing per comment 6?  Which ones are succeeding?

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to comment #12)
> Which controls are failing per comment 6?  

type=file and type=hidden

> Which ones are succeeding?

type=radio and <select>. Also, type=password is successfully forgotten but it could get forgotten for the wrong reason.

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: Flush notifications on input and button, make Terminate() break out of update batch, refactor other stuff around update batch.

(In reply to comment #12)
> (which, note, flushes the
> sink notifications in the old parser; this is a known suck issue)

Thanks! I made <input> and </button> flush append notifications, and now the test case passes.

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 411656 [details] [diff] [review]
Flush notifications on input and button, make Terminate() break out of update batch, refactor other stuff around update batch.

(It may be currently unnecessary to reopen the update batch when a charset switch fails, but it seems safer to reopen it in all break-outs in case subsequent patches change assumptions on what tree ops always occur as the last of of a queue.)

Comment 16

[Boris Zbarsky [:bzbarsky]]

> it could get forgotten for the wrong reason.

Right.  So it sounds like the things outside the form (except the <select> for some reason) are failing.  If you move those controls into the form, do they start succeeding?

> Thanks! I made <input> and </button> flush append notifications

So you're basically flushing only for things that want a DoneCreatingElement and doing so before the DoneCreatingElement call happens?  I guess only <input> and <button> implement DoneCreatingElement?

Comment 17

[Boris Zbarsky [:bzbarsky]]

It might be worth investigating other places that flush the sink to see whether they need some sort of changes to the batching the new parser does...

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to comment #16)
> If you move those controls into the form, do they
> start succeeding?

I haven't tested yet.

> > Thanks! I made <input> and </button> flush append notifications
> 
> So you're basically flushing only for things that want a DoneCreatingElement
> and doing so before the DoneCreatingElement call happens?

Yes..

> I guess only <input> and <button> implement DoneCreatingElement?

Correct.

(In reply to comment #17)
> It might be worth investigating other places that flush the sink to see whether
> they need some sort of changes to the batching the new parser does...

An alternative to flushing as a side effect of DoneCreatingElement would be making the tree op executor flush pending append notifications when it gets a flush request via the old API and mFlushState == eInDocUpdate.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm...  I'd been hoping to eliminate the flush api, myself.

Would it work to move form state restoration to a script runner, so it happens after the update is done, and not worry about messing with the update here?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Jonas, thoughts on comment 19?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, that is the way I think we should do it. The way I envision things to work is this:

Before starting execute a batch of nsHtml5TreeOperations, call BeginUpdate
As nodes are inserted into the DOM, notify, but do this in as big chunks as possible.
When done executing the batch of nsHtml5TreeOperations, call EndUpdate

So by the time we call EndUpdate we have notified on all the nodes that were inserted into the tree.

No scripts should execute between the BeginUpdate and EndUpdate. Anything that wants do execute scripts needs to use a nsScriptRunner. These scriptrunners will be run by EndUpdate removing the last scriptblocker, by which time we've notified on all inserted content.

That should make this situation work, right?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, for what it's worth, once we do the above, it should be easy to change from notification-batching to insertion batching...

Comment 23

[Henri Sivonen (:hsivonen)]

(In reply to comment #21)
> Yes, that is the way I think we should do it.

This being what the patch does, this being reusing the old content-initiated flush API or this being removing the old content-initiated flush API but compensating for lack thereof in a way different from the patch?

> The way I envision things to work
> is this:
> 
> Before starting execute a batch of nsHtml5TreeOperations, call BeginUpdate

This happens already.

> As nodes are inserted into the DOM, notify, but do this in as big chunks as
> possible.

Append child notifications are deferred. Other notifications aren't. The append notification batches try to be as big as possible, although I supposed there are some cases where intermediate notification flushes happen according to worst-case scenario and it might be possible to avoid flushing if I knew more about the scenarios.

> When done executing the batch of nsHtml5TreeOperations, call EndUpdate

This happens already.

> So by the time we call EndUpdate we have notified on all the nodes that were
> inserted into the tree.

This happens already.

> No scripts should execute between the BeginUpdate and EndUpdate. Anything that
> wants do execute scripts needs to use a nsScriptRunner. These scriptrunners
> will be run by EndUpdate removing the last scriptblocker, by which time we've
> notified on all inserted content.

This already happens. However, style linking elements end and reopen the update in order to extinguish a useless script runner and failed charset switches end and reopen the update, because successful charset switches cannot happen inside a batch (and success/failure is discovered by trying to perform the switch).

> That should make this situation work, right?

No, that's not enough. For content/base/test/test_bug345339.html to pass, the append notifications for <input> need to be flushed early.

(In reply to comment #22)
> Also, for what it's worth, once we do the above, it should be easy to change
> from notification-batching to insertion batching...

What kind of insertion batching do you mean? The tree op queue flush is a kind of insertion batch.

Comment 24

[Henri Sivonen (:hsivonen)]

(In reply to comment #16)
> If you move those controls into the form, do they start succeeding?

Yes. I'll tweak the patch accordingly.

Comment 25

[Henri Sivonen (:hsivonen)]

Attachment: Flush notifications only when the form pointer is null

Differences from the previous patch:
 * Notifications are flushed only when the form input isn't about to end up being associated with a form.
 * Added comments in nsHtml5TreeBuilderCppSupplement.h and nsContentUtils.cpp making it possible to track the relevant lines of code back to this bug to reduces the obscurity of why things are done the way they are done.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #23)
> (In reply to comment #21)
> > Yes, that is the way I think we should do it.
> 
> This being what the patch does, this being reusing the old content-initiated
> flush API or this being removing the old content-initiated flush API but
> compensating for lack thereof in a way different from the patch?
> 
> > The way I envision things to work
> > is this:
> > 
> > Before starting execute a batch of nsHtml5TreeOperations, call BeginUpdate
> 
> This happens already.
> 
> > As nodes are inserted into the DOM, notify, but do this in as big chunks as
> > possible.
> 
> Append child notifications are deferred. Other notifications aren't. The append
> notification batches try to be as big as possible, although I supposed there
> are some cases where intermediate notification flushes happen according to
> worst-case scenario and it might be possible to avoid flushing if I knew more
> about the scenarios.

Surely you sometimes need to defer insert notifications too, i.e. in the following markup:

<div>
<p>
</p>
<table>
<tr>
<span>
...
</div>

Say that the <div> has already been parsed and notified on. You'll then parse and insert the <p> and the <table> without notifying. Do you then notify when inserting the <span> before the <table>? If so, do you flush the other notifications first?

> > That should make this situation work, right?
> 
> No, that's not enough. For content/base/test/test_bug345339.html to pass, the
> append notifications for <input> need to be flushed early.

Even if form restoration happens from a scriptrunner? And thus after all notifications have happened?

(I assumed form restoration can run "change" events and the like)

Comment 27

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 412832 [details] [diff] [review]
Flush notifications only when the form pointer is null

Looks good to me.

Just curious: why is there not a method called nsHtml5TreeOpExecutor::DisableFragmentMode, and why do we not call this method at the end of nsHtml5Parser::ParseFragment (to complement the call to nsHtml5TreeOpExecutor::EnableFragmentMode earlier in ParseFragment)?  I understand that nsHtml5TreeOpExecutor::mFragmentMode is reset in nsHtml5TreeOpExecutor::Reset, which is called from nsHtml5Parser::Reset, but nsHtml5Parser::Reset is only called externally, from nsContentUtils::CreateContextualFragment.  If resetting the fragment mode is important, we might want to have the parser do it instead of relying on external code.

r=me provided that's not a source of concern.

Note to other reviewers: tracing the lifecycle of mFlushState is a good way to get started reading this patch.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I do still think that we should move restoration to a scriptrunner.  The flushes that this code performs have some nasty performance effects, last I checked.  Maybe Henri's code doesn't have those problems, but it would be easier to make sure if we just didn't add parser hackery to work around the content behavior here.

Comment 29

[Henri Sivonen (:hsivonen)]

(In reply to comment #26)
> > > As nodes are inserted into the DOM, notify, but do this in as big chunks as
> > > possible.
> > 
> > Append child notifications are deferred. Other notifications aren't. The append
> > notification batches try to be as big as possible, although I supposed there
> > are some cases where intermediate notification flushes happen according to
> > worst-case scenario and it might be possible to avoid flushing if I knew more
> > about the scenarios.
> 
> Surely you sometimes need to defer insert notifications too, i.e. in the
> following markup:
> 
> <div>
> <p>
> </p>
> <table>
> <tr>
> <span>
> ...
> </div>
> 
> Say that the <div> has already been parsed and notified on. You'll then parse
> and insert the <p> and the <table> without notifying. Do you then notify when
> inserting the <span> before the <table>? If so, do you flush the other
> notifications first?

Before the insertion of <span> proceeds, it flushes the appends for <p>, <table> and <tr>. The insertion of the <span> then happens and is notified immediately.

Basically, foster-parenting and the residual style stuff are notification batching hazards that fall off the fast track. At least those pieces of badness-causing syntax are non-conforming.

(<style>, <link> and </head> are notification batching hazards, too. However, <style> and <link> would be easier to fix into non-hazards than foster-parenting and residual style. </head> starts layout and there was some reason why I thought notifications need to be flushed when starting layout. I can't remember what that reason was.)

> > > That should make this situation work, right?
> > 
> > No, that's not enough. For content/base/test/test_bug345339.html to pass, the
> > append notifications for <input> need to be flushed early.
> 
> Even if form restoration happens from a scriptrunner? And thus after all
> notifications have happened?
> 
> (I assumed form restoration can run "change" events and the like)

The part of the patch that changes the way nsIParser::Terminate() cuts through everything fixed the restoration part.

The flushing issue is about saving the form state in the first place.

(In reply to comment #27)
> Just curious: why is there not a method called
> nsHtml5TreeOpExecutor::DisableFragmentMode, and why do we not call this method
> at the end of nsHtml5Parser::ParseFragment (to complement the call to
> nsHtml5TreeOpExecutor::EnableFragmentMode earlier in ParseFragment)? I
> understand that nsHtml5TreeOpExecutor::mFragmentMode is reset in
> nsHtml5TreeOpExecutor::Reset, which is called from nsHtml5Parser::Reset, but
> nsHtml5Parser::Reset is only called externally, from
> nsContentUtils::CreateContextualFragment.  If resetting the fragment mode is
> important, we might want to have the parser do it instead of relying on
> external code.

The reliance of particular Reset() and ParseFragment() usage patterns is badness inherited from nsParser. I filed bug 529501 about this.

(I've tried to keep Reset() doing what it says it does in order to make it easier to change the external assumptions later, which is why it does some useless resetting as it does here.)

> r=me provided that's not a source of concern.

Thank you.

(In reply to comment #28)
> Maybe Henri's code doesn't have those problems, but it would be
> easier to make sure if we just didn't add parser hackery to work around the
> content behavior here.

The problem is that if every issue uncovered as part of the HTML5 parser work is fixed the Right Way at the source of the problem, it's even harder to close the gap between the old parser and the HTML5 parser and get the HTML5 parser turned on by default. During the time of fixing the known problems the Right Way before turning the HTML5 parser on by default, something else gets landed that doesn't work right with the HTML5 parser and the goalposts keep escaping. Therefore, I suggest landing this patch as is (now with comments that explain what the hackery is for) and then fixing the problem at the source the Right Way as part of bug 529503 that doesn't need to be on the critical path for turning the HTML5 parser on by default.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> fixing the problem at the source the Right Way as part of bug 529503

Sounds like a plan, as long as we don't lose that bug...  Can we make it dependent on html5 parser turn-on, please, so we track it?

Comment 31

[Henri Sivonen (:hsivonen)]

(In reply to comment #30)
> > fixing the problem at the source the Right Way as part of bug 529503
> 
> Sounds like a plan, as long as we don't lose that bug...  Can we make it
> dependent on html5 parser turn-on, please, so we track it?

I made it a "See also" on the HTML5 parser tracking bug, since, strictly speaking, they could be fixed in either order.

Comment 32

[Henri Sivonen (:hsivonen)]

http://hg.mozilla.org/mozilla-central/rev/334f57fa1ade

Comment 33

[Boris Zbarsky [:bzbarsky]]

This actually made us flush in all sorts of cases where the old parser used to not flush.... See bug 534458.