448564 - (CVE-2008-3444) Radware DoS report [@ nsSubDocumentFrame::Reflow]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: radware testcase

Radware reported a DoS crash on the attached testcase

<s>
<form>a</form>
<iframe></iframe>
<script src=a></script>
<form></form>
<table>
<optgroup>

I have not been able to reproduce the crash on Mac or Windows with a 3.0.1 or thereabouts build (nor could jst in 3.0), but the reported stack is a null dereference:

Unhandled exception at 0x606fe7b1 (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x00000004.
 
>          xul.dll!nsSubDocumentFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 507 + 0xb bytes            C++
            xul.dll!nsLineLayout::ReflowFrame(nsIFrame * aFrame=0x0258d8b8, unsigned int & aReflowStatus=0x00000000, nsHTMLReflowMetrics * aMetrics=0x00000000, int & aPushedFrame=0x00000000)  Line 857          C++
            xul.dll!nsInlineFrame::ReflowInlineFrame(nsPresContext * aPresContext=0x00b9dae0, const nsHTMLReflowState & aReflowState={...}, nsInlineFrame::InlineReflowState & irs={...}, nsIFrame * aFrame=0x00000000, unsigned int & aStatus=0x00000000)  Line 611  C++
            xul.dll!nsInlineFrame::ReflowFrames(nsPresContext * aPresContext=0x00b9dae0, const nsHTMLReflowState & aReflowState={...}, nsInlineFrame::InlineReflowState & irs={...}, nsHTMLReflowMetrics & aMetrics={...}, unsigned int & aStatus=0x00000000)  Line 479  C++
            xul.dll!nsInlineFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 399   C++
            xul.dll!nsLineLayout::ReflowFrame(nsIFrame * aFrame=0x0258d848, unsigned int & aReflowStatus=0x00000000, nsHTMLReflowMetrics * aMetrics=0x00000000, int & aPushedFrame=0x00000000)  Line 857          C++
            xul.dll!nsBlockFrame::ReflowInlineFrame(nsBlockReflowState & aState={...}, nsLineLayout & aLineLayout={...}, nsLineList_iterator aLine={...}, nsIFrame * aFrame=0x0258d848, LineReflowStatus * aLineReflowStatus=0x0012e29c)  Line 3608            C++
            xul.dll!nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & aState={...}, nsLineLayout & aLineLayout={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012e4c4, LineReflowStatus * aLineReflowStatus=0x0012e388, int aAllowPullUp=0x00000001)  Line 3430     C++
            xul.dll!nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012e4c4)  Line 3279        C++
            xul.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012e4c4)  Line 2335 + 0x17 bytes            C++
            xul.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...})  Line 1898        C++
            xul.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 938   C++
            xul.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000001, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000000, nsMargin & aComputedOffsets={...}, nsLineBox * aLine=0x0258db50, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000, nsBlockReflowState & aState={...})  Line 339 + 0x24 bytes        C++
            xul.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012eb64)  Line 3023        C++
            xul.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012eb64)  Line 2282 + 0xf bytes   C++
            xul.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...})  Line 1898        C++
            xul.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 938   C++
            xul.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000001, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000001, nsMargin & aComputedOffsets={...}, nsLineBox * aLine=0x022a358c, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000, nsBlockReflowState & aState={...})  Line 339 + 0x24 bytes        C++
            xul.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012f204)  Line 3023         C++
            xul.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012f204)  Line 2282 + 0xf bytes    C++
            xul.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...})  Line 1898        C++
            xul.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 938   C++
            xul.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x00000000, nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0x00000000, int aY=0x00000000, unsigned int aFlags=0x00000000, unsigned int & aStatus=, nsOverflowContinuationTracker * aTracker=0x00000000)  Line 755 + 0x13 bytes     C++
            xul.dll!CanvasFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 589      C++
            xul.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x00000000, nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0x00000000, int aY=0x00000000, unsigned int aFlags=0x00000003, unsigned int & aStatus=, nsOverflowContinuationTracker * aTracker=0x00000000)  Line 755 + 0x13 bytes     C++
            xul.dll!nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState * aState=0x0012f808, int aAssumeHScroll=0x00000000, int aAssumeVScroll=0x00000000, nsHTMLReflowMetrics * aMetrics=0x0012f720, int aFirstPass=0x0012f7b8)  Line 493           C++
            xul.dll!nsHTMLScrollFrame::ReflowContents(ScrollReflowState * aState=0x0012f808, const nsHTMLReflowMetrics & aDesiredSize={...})  Line 570       C++
            xul.dll!nsHTMLScrollFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 771  C++
            xul.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x00000000, nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0x00000000, int aY=0x00000000, unsigned int aFlags=0x00000000, unsigned int & aStatus=, nsOverflowContinuationTracker * aTracker=0x00000000)  Line 755 + 0x13 bytes     C++
            xul.dll!ViewportFrame::Reflow(nsPresContext * aPresContext=0x00b9dae0, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000)  Line 288      C++
            xul.dll!PresShell::DoReflow(nsIFrame * target=0x00000000)  Line 6215       C++
            xul.dll!PresShell::ProcessReflowCommands(int aInterruptible=0x00000001)  Line 6307        C++
            xul.dll!PresShell::DoFlushPendingNotifications(mozFlushType aType=Flush_Layout, int aInterruptibleReflow=0x00000001)  Line 4513        C++
            xul.dll!PresShell::ReflowEvent::Run()  Line 6067  
            xul.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012fc68)  Line 511    C++
            xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00000000, int mayWait=0x0012ff14)  Line 227 + 0xc bytes     C++
            xul.dll!nsBaseAppShell::Run()  Line 153   C++
            xul.dll!nsAppStartup::Run()  Line 182       C++
            xul.dll!XRE_main(int argc=0x00000001, char * * argv=0x00349bc8, const nsXREAppData * aAppData=0x00349f60)  Line 3137     C++
            firefox.exe!NS_internal_main(int argc=0x00000001)  Line 159  C++
            firefox.exe!wmain(int argc=0x00000001, wchar_t * * argv=0x00347c90)  Line 55 + 0x8 bytes            C++
            firefox.exe!__tmainCRTStartup()  Line 594 + 0x17 bytes    C
            kernel32.dll!_BaseProcessStart@4()  + 0x23 bytes           

Source nsframeframe.cpp, line 507:
  mInnerView is 0x00;
  nsIViewManager* vm = mInnerView->GetViewManager();

Comment 1

[Johnny Stenback (:jst)]

When loading the testcase (from bugzilla, loading from local file doesn't show the crash) I hit a couple of assertions that don't look related, and then I hit the assertion:

  NS_ASSERTION(aPresContext->GetPresShell()->GetPrimaryFrameFor(mContent) == this,
               "Shouldn't happen");

in nsSubDocumentFrame::Reflow(), and then after that we cash in nsIView::GetViewManager() because this is null.

Comment 2

[Martijn Wargers (dead)]

This regressed between 2006-04-10 and 2006-04-11:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-10+04&maxdate=2006-04-11+09&cvsroot=%2Fcvsroot
My guess is a regression form bug 332927, somehow.

Comment 3

[Martijn Wargers (dead)]

Maybe some of the comments in bug 369547 are useful for this?

Comment 4

[Boris Zbarsky [:bzbarsky]]

This looks like a parser/content-sink bug.  We're getting a ContentAppended on the body aIndexInContainer == 2, coming from nsHTMLContentSink::CloseBody.  At this point, the relevant part of the DOM looks like this (containment indicated by nesting):

  <body>
    <s>
      #text
    <form>
      <select>
        <optgroup>
          #text
    <form>
      <s>
        #text
        <iframe>
        #text
        <script>
        #text
      <form>
      #text
      <table>
        #text

Thing is, when we last notified (from HandleSavedTokens calling BeginContext), that second form wasn't there, and the third one was.  So in other words, we're double-notifying on the entire subtree rooted at that third form, which naturally breaks things.

Somewhere in here we should have had a ContentInserted notification (when we created the <form> containing the <select>), and updated the mNumFlushed for the <body> accordingly.

Comment 5

[Jesse Ruderman]

This is a null deref, right?  Any reason for it to be security-sensitive?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Given comment 4, it should be possible to trigger other (not just null-deref) crashes here, I think.  The frame tree is just badly broken.

Comment 7

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

Given that, i'm gonna set some flags to up the priority on this one.

I'll take this for now, but if someone wants to, feel free to take it. I won't be able to look at this for at least a couple of weeks.

Comment 8

[Johnny Stenback (:jst)]

cc:ing Ben Newman who said he could help look into this. Ben, in this case we end up creating two nsSubDocumentFrame's (and other frames too) for a single DOM node, the construction of the frames happens off of notifications from the content sink (ContentAppended() most likely, in either nsHTMLContentSink or nsContentSink). We should only ever get one notification for any given DOM node being inserted into the DOM tree, but in this case we most likely fire two of them, and thus end up with the duplicated frames etc.

Feel free to take this bug if you'll be able to get to it before Jonas does...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ben, see comment 4 for some details of when the incorrect notifications happen...  the real question is what mutated the DOM without updating the notification indices in the sink.

Comment 10

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

(In reply to comment #1)
> When loading the testcase (from bugzilla, loading from local file doesn't show
> the crash) I hit a couple of assertions that don't look related, and then I hit
> the assertion:
> 
>   NS_ASSERTION(aPresContext->GetPresShell()->GetPrimaryFrameFor(mContent) ==
> this,
>                "Shouldn't happen");
> 
> in nsSubDocumentFrame::Reflow(), and then after that we cash in
> nsIView::GetViewManager() because this is null.
> 

For what it's worth, this particular point of failure can be blamed on the changeset Martijn identified above:

https://bugzilla.mozilla.org/attachment.cgi?id=217746&action=diff#mozilla/layout/generic/nsFrameFrame.cpp_sec3

I see no reason not to reinstate the null check, but the problem runs deeper than that.

Comment 11

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

We (or at least I :) ) generally don't add nullchecks in cases when something "is fairly certain never to be null and we know of case where it happens, and if it does much worse things are going to fail anyway". Otherwise we end up nullchecking ourselves to death.

However I'm not sure if that is the case here or not. Obviously it does happen that it is null sometimes :)

Comment 12

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

(In reply to comment #11)
> if it does much worse things are going to fail anyway

I definitely think this passes the "bigger problems elsewhere" test :)  I'm okay with leaving the null check out.

Comment 13

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

Small update:

Running under gdb sometimes triggers the correct behavior.  After a few rounds of binary-search debugging, I isolated the last instruction I can break on without altering normal notification behavior.  In other words, if I step over this instruction and then continue, the page renders just fine.

The instruction is mLastNotificationTime = PR_Now(), at nsContentSink.cpp:1087

Setting mLastNotificationTime to 0, which makes it appear that the last notification hasn't happened since 1970, is a hacky way to suppress the problem.  This may or may not represent any progress; in any case, I need to make sure that a notification gets fired deterministically, rather than depending on periodical notifications.

Comment 14

[Boris Zbarsky [:bzbarsky]]

You could turn off notifications completely in the sink (there are prefs for it), then trigger notifications by tossing <script> nodes into the DOM.  Especially if the script just does |document.body.offsetWidth|

Comment 15

[Daniel Veditz [:dveditz]]

Looks like this has been assigned a CVE number
http://nvd.nist.gov/nvd.cfm?cvename=CVE-2008-3444

Comment 16

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

Attachment: proposed patch with (very tentative) test cases

The DOM mutation that wasn't being followed by a notification was the call to mSink->InsertChildAt in SinkContext::OpenContainer.  Mimicking other parts of the content sink code, I added a call to DidAddContent at the end of OpenContainer.  Then I realized that DidAddContent could be made to handle NotifyAppend as well as NotifyInsert.  With that change, several other chunks of code became redundant (specifically, code calling both NotifyAppend and OpenContainer/DidAddContent).  The only sticky part was in OpenFrameset, but I moved that logic into the switch statement at the end of OpenContainer.

Since NotifyAppend and NotifyInsert are now called in OpenContainer, CloseContainer has less work to do.  It still calls DidAddContent, but this call rarely if ever results in a notification.  This is a good thing, I think, because the order of CloseContainer calls is the reverse of the order in which we want to be notifying.  If I am not mistaken, the purpose of mNotifyLevel was to reduce the number of notifications stemming from these CloseContainer calls, so I believe it's now safe to remove the mNotifyLevel altogether.

If any of my assumptions are off the mark, those last two paragraphs may be complete nonsense.  I look forward to your feedback :)

One big question: I have no idea how to test this stuff, because I'm not sure how to detect double rendering using mochitest, given that the DOM gets constructed just fine.  Nevertheless, my patch includes the mochitests that I used during my debugging.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Ben, it sounds like you want to test this using a reftest instead of a mochitest.

Comment 18

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

I'd say you should test with both, while watching out for any assertions that this might trigger. Not sure if we have any good assertions in there currently, or if new ones should be added.

Comment 19

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

(In reply to comment #17)
> Ben, it sounds like you want to test this using a reftest instead of a
> mochitest.

From http://developer.mozilla.org/en/docs/Creating_reftest-based_unit_tests:

"So, if the effect of complex markup is being tested, put that complex markup into a page and create another page that uses simple markup to achieve the same visual effect. Reftest will then compare them and verify whether they produce the same visual construct."

That's just what I need.  Thanks!

Comment 20

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

Ah, sorry, to write a regression test using reftest sounds like the right choise. But to make sure you don't regress anything else follow the steps in my last comment :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

This can be mochitested too, I bet.  getElementsByTagName("iframe") up front, get its length, then finish parsing, then get its length again.  With the old code it might well be off....

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'll try to get to this review tomorrow.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Gah.  Sorry for the lag... So in this case, mStack[mStackPos - 2].mInsertionPoint != -1 but mStack[mStackPos-2].mNumFlushed > mStack[mStackPos - 2].mInsertionPoint ?  That's the only way I can see for the notifications to get confused by the InsertChildAt call, right?

Comment 24

[Boris Zbarsky [:bzbarsky]]

So looking at the patch, DidAddContent will now eagerly notify any time mNumFlushed < GetChildCount().  That's wrong.  We want to limit the eager notification to the insert case, and in the usual append case we want to not do it.  I suspect that this patch as written leads to a significant pageload regression.  The fact that the "walk up the tree and notify" code in CloseContainer could be removed is a result of this eager notification, I assume?

Also, the check for IsTimeToNotify() needs to stay in CloseContainer(), ideally: we don't really want to be constructing frames as we open containers, but rather as we close them.  Again, for performance.

Did the notifications on body/frameset get to go away because DidAddContent is now being called on those opens instead or something?

Thinking back to the cause of this bug, is the problem that we end up flushing out the sink or notifying between the OpenContainer (which adds the node to the DOM) and the CloseContainer (which notifies the insert), and as a result flush totally bogus information?

It seems like the right solution would be to notify eagerly on insert (either when inserting before mNumFlushed, or in general; the latter is probably easier since it doesn't involve keeping track of whether you still need to notify on the insert), not to switch to eager notifications for everything.

Comment 25

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

(In reply to comment #24)
> It seems like the right solution would be to notify eagerly on insert (either
> when inserting before mNumFlushed, or in general; the latter is probably easier
> since it doesn't involve keeping track of whether you still need to notify on
> the insert), not to switch to eager notifications for everything.

Notifying after InsertChildAt in OpenContainer (but not after AppendChildTo) almost works, except that notifications do not happen for children of the inserted child.  I assume this is because the children haven't been appended yet, because the notification is happening in OpenContainer.  I need to figure out why the parent of the InsertChildAt call appears fully flushed when it gets closed in CloseContainer.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yeah, if we close an already-notified-on child we need to notify on its kids.  There's code in CloseContainer that does that now, and would need to be called even in this situation.

Comment 27

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

Update / note to self.  Looks like we've lost the InsertChildAt insertion point by the time we FlushTags in CloseBody.  In other words, we can tell that the parent has a new child, but not which child is new (or, rather, we assume we appended and thus double-notify for the last child).  The problem is that mCurrentContext has changed since the insertion happened, and the stack node corresponding to the parent in the new context is distinct from the stack node whose insertion point we updated when we called InsertChildAt.  The mContent fields of the two stack nodes point to the same object, but the mInsertionPoint fields are inconsistent.

Comment 28

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

(In reply to comment #27)
> Update / note to self.  Looks like we've lost the InsertChildAt insertion point
> by the time we FlushTags in CloseBody.  In other words, we can tell that the
> parent has a new child, but not which child is new (or, rather, we assume we
> appended and thus double-notify for the last child).  The problem is that
> mCurrentContext has changed since the insertion happened, and the stack node
> corresponding to the parent in the new context is distinct from the stack node
> whose insertion point we updated when we called InsertChildAt.  The mContent
> fields of the two stack nodes point to the same object, but the mInsertionPoint
> fields are inconsistent.

Is it reasonable to have more than one context whose stack contains a particular node?  If not, then I need to figure out how the duplication happened.  If so, then it seems problematic that the mContent fields of different nodes are aliases but the integer fields are not.

Comment 29

[Boris Zbarsky [:bzbarsky]]

I would have thought we'd flush all pending notifications when switching contexts...

And yes, you can definitely have multiple contexts which all have the <body> on the stack.  Peter, Jonas, or Blake may know more; I've never really understood the details of the sinkcontext stuff, to be honest.

Comment 30

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

Attachment: new patch, same lousy test cases

This patch should be a lot easier to review.  The trick was to back-propagate mInsertionPoint when closing a context.  mNumFlushed was already getting propagated back, but no such love for mInsertionPoint.  This exposed a bug in FlushTags: the inserted child is not necessarily the next node on the stack, but since we know the insertion point, it's easy enough to determine which child was actually inserted.

Going to start working on proper reftests as soon as I post this patch.

Comment 31

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

Attachment: same patch, spiffy new reftests

2 reftests, 1 crashtest

Comment 32

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

(In reply to comment #30)
> This patch should be a lot easier to review.

s/should be a lot easier to review/is considerably simpler/

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hmm.  What happens if we have inserted multiple kids at mInsertionPoint?  Or are we guaranteed to have notified on all but one of them?

Also, when would the child on the stack not be the one at the insertion point?  

Here's what I worry about.  The following sequence of events:

1)  Append two kids.  Notify on them.
2)  Insert a kid at position 1 (between the two).  Don't notify.
3)  Append another kid

Can this happen?  Or are we guaranteed to notify on the inserted kids if mInsertionPoint becomes -1 again?

I really wish someone who knows this insertion point stuff better would chime in here... ;)

Comment 34

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

(In reply to comment #33)
> Hmm.  What happens if we have inserted multiple kids at mInsertionPoint?  Or
> are we guaranteed to have notified on all but one of them?

Bad times.  I suspect the author of the content sink intended to make such a guarantee, but it's implicit at best.  Code like the following appears in multiple places:

    if (parent.numFlushed < childCount) {
        notify(parent, child, ...);
        parent.numFlushed = childCount;
    }

This assumes childCount - parent.numFlushed <= 1, since the notification only flushes one child. This assumption probably does warrant investigation, even if it's not directly responsible for the current bug.

> Also, when would the child on the stack not be the one at the insertion point?

As far as I can tell, this would never happen if HTMLContentSink::EndContext called FlushTags, but it doesn't, so it's possible for nodes to get inserted in a new context but notified only after the context is closed.  Such nodes won't be on the stack in the old context, but their parents might be.

> Here's what I worry about.  The following sequence of events:
> 
> 1)  Append two kids.  Notify on them.
> 2)  Insert a kid at position 1 (between the two).  Don't notify.
> 3)  Append another kid
> 
> Can this happen?  Or are we guaranteed to notify on the inserted kids if
> mInsertionPoint becomes -1 again?

I share this concern.  There may be an argument that two children are never inserted/appended into the same parent without a notification, but we are definitely hosed if that happens, since we don't keep track of enough information to recover.  If we want to enforce this invariant explicitly, we have to do it at insertion-time (when we're about to insert/append a child, make sure the parent is fully flushed).

Comment 35

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

(In reply to comment #33)
> Hmm.  What happens if we have inserted multiple kids at mInsertionPoint?  Or
> are we guaranteed to have notified on all but one of them?

I added a method to the SinkContext::Node struct to handle all insertions/appendings and also to check whether multiple children are ever inserted or appended without first flushing the parent.  We append multiple children without flushing quite frequently, which seems okay because we can later assume all the unflushed children are at the end of the child list.  But your question still stands: do multiple *insertions* ever happen without intervening notifications?  I don't have direct evidence one way or the other, but it would be easy to prevent multiple insertions preemptively.

Maybe someone familiar with the content sink can comment on the necessity of such preemptive measures...

Comment 36

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

Attachment: decomposed SinkContext::Node::Add

Simplify/centralize child addition logic.  Add an assertion to ensure we only insert into fully flushed nodes.

Comment 37

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

Attachment: FlushTags in HTMLContentSink::EndContext

Now calling FlushTags in EndContext, both to be more symmetrical with BeginContext (call FlushTags whenever switching contexts) and to avoid having to worry about flushing children not on the stack in FlushTags and DidAddContent.  Assertions now verify that inserted children are in the expected place on the stack.

Also added another reftest to catch a double rendering bug that I concocted earlier this afternoon.

Comment 38

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

Comment on attachment 335973 [details] [diff] [review]
FlushTags in HTMLContentSink::EndContext

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/content/html/document/crashtests/448564.html b/content/html/document/crashtests/448564.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/crashtests/448564.html
>@@ -0,0 +1,7 @@
>+<s>
>+<form>a</form>
>+<iframe></iframe>
>+<script src=a></script>
>+<form></form>
>+<table>
>+<optgroup>
>\ No newline at end of file
>diff --git a/content/html/document/crashtests/crashtests.list b/content/html/document/crashtests/crashtests.list
>--- a/content/html/document/crashtests/crashtests.list
>+++ b/content/html/document/crashtests/crashtests.list
>@@ -1,3 +1,4 @@ load 388183-1.html
> load 388183-1.html
> load 395340-1.html
> load 407053.html
>+load 448564.html
>diff --git a/content/html/document/reftests/bug448564-1_malformed.html b/content/html/document/reftests/bug448564-1_malformed.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/reftests/bug448564-1_malformed.html
>@@ -0,0 +1,11 @@
>+<html>
>+<body>
>+  <script src=></script>
>+  <b>
>+    <form>a</form>
>+    <form>b</form>
>+    <table>
>+    <optgroup></optgroup>
>+  </b>
>+</body>
>+</html>
>\ No newline at end of file
>diff --git a/content/html/document/reftests/bug448564-1_well-formed.html b/content/html/document/reftests/bug448564-1_well-formed.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/reftests/bug448564-1_well-formed.html
>@@ -0,0 +1,13 @@
>+<html>
>+<body>
>+  <b>a</b>
>+  <form>
>+    <select>
>+      <optgroup />
>+    </select>
>+  </form>
>+  <form>
>+    <b>b</b>
>+  </form>
>+</body>
>+</html>
>\ No newline at end of file
>diff --git a/content/html/document/reftests/bug448564-2_malformed.html b/content/html/document/reftests/bug448564-2_malformed.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/reftests/bug448564-2_malformed.html
>@@ -0,0 +1,11 @@
>+<html>
>+<body>
>+  <s>
>+  <form>a</form>
>+  <iframe></iframe>
>+  <script src=a></script>
>+  <form></form>
>+  <table>
>+  <optgroup>
>+</body>
>+</html>
>\ No newline at end of file
>diff --git a/content/html/document/reftests/bug448564-2_well-formed.html b/content/html/document/reftests/bug448564-2_well-formed.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/reftests/bug448564-2_well-formed.html
>@@ -0,0 +1,15 @@
>+<html>
>+<body>
>+  <form>
>+    <select>
>+      <optgroup />
>+    </select>
>+  </form>
>+  <form>
>+    <s>
>+      a
>+      <iframe></iframe>
>+    </s>
>+  </form>
>+</body>
>+</html>
>\ No newline at end of file
>diff --git a/content/html/document/reftests/bug448564-3_malformed.html b/content/html/document/reftests/bug448564-3_malformed.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/reftests/bug448564-3_malformed.html
>@@ -0,0 +1,4 @@
>+<table>
>+  <th>head</th>
>+  <optgroup>b</optgroup>
>+</table>
>\ No newline at end of file
>diff --git a/content/html/document/reftests/bug448564-3_well-formed.html b/content/html/document/reftests/bug448564-3_well-formed.html
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/reftests/bug448564-3_well-formed.html
>@@ -0,0 +1,8 @@
>+<form>
>+  <select>
>+    <optgroup>b</optgroup>
>+  </select>
>+</form>
>+<table>
>+  <th>head</th>
>+</table>
>\ No newline at end of file
>diff --git a/content/html/document/reftests/reftests.list b/content/html/document/reftests/reftests.list
>new file mode 100644
>--- /dev/null
>+++ b/content/html/document/reftests/reftests.list
>@@ -0,0 +1,3 @@
>+== bug448564-1_well-formed.html bug448564-1_malformed.html
>+== bug448564-2_well-formed.html bug448564-2_malformed.html
>+== bug448564-3_well-formed.html bug448564-3_malformed.html
>diff --git a/content/html/document/src/nsHTMLContentSink.cpp b/content/html/document/src/nsHTMLContentSink.cpp
>--- a/content/html/document/src/nsHTMLContentSink.cpp
>+++ b/content/html/document/src/nsHTMLContentSink.cpp
>@@ -368,16 +368,18 @@ public:
>   nsCOMPtr<nsIContent> mLastTextNode;
>   PRInt32 mLastTextNodeSize;
> 
>   struct Node {
>     nsHTMLTag mType;
>     nsGenericHTMLElement* mContent;
>     PRUint32 mNumFlushed;
>     PRInt32 mInsertionPoint;
>+    
>+    nsIContent *Add(nsIContent *child);
>   };
> 
>   Node* mStack;
>   PRInt32 mStackSize;
>   PRInt32 mStackPos;
> 
>   PRUnichar* mText;
>   PRInt32 mTextLength;
>@@ -725,20 +727,22 @@ SinkContext::DidAddContent(nsIContent* a
>       SINK_TRACE(gSinkLogModuleInfo, SINK_TRACE_REFLOW,
>                  ("SinkContext::DidAddContent: Insertion notification for "
>                   "parent=%s at position=%d and stackPos=%d",
>                   str.get(), mStack[mStackPos - 1].mInsertionPoint - 1,
>                   mStackPos - 1));
>     }
> #endif
> 
>-    mSink->NotifyInsert(parent, aContent,
>-                        mStack[mStackPos - 1].mInsertionPoint - 1);
>+    PRInt32 childIndex = mStack[mStackPos - 1].mInsertionPoint - 1;
>+    NS_ASSERTION(parent->GetChildAt(childIndex) == aContent,
>+                 "Flushing the wrong child.");
>+    mSink->NotifyInsert(parent, aContent, childIndex);
>     mStack[mStackPos - 1].mNumFlushed = parent->GetChildCount();
>-  } else if (mSink->IsTimeToNotify()) {
>+  } else if (false && mSink->IsTimeToNotify()) {
>     SINK_TRACE(gSinkLogModuleInfo, SINK_TRACE_REFLOW,
>                ("SinkContext::DidAddContent: Notification as a result of the "
>                 "interval expiring; backoff count: %d", mSink->mBackoffCount));
>     FlushTags();
>   }
> }
> 
> nsresult
>@@ -830,25 +834,17 @@ SinkContext::OpenContainer(const nsIPars
>       break;
>     default:
>       break;    
>   }
>   
>   rv = mSink->AddAttributes(aNode, content);
>   MaybeSetForm(content, nodeType, mSink);
> 
>-  nsGenericHTMLElement* parent = mStack[mStackPos - 2].mContent;
>-
>-  if (mStack[mStackPos - 2].mInsertionPoint != -1) {
>-    parent->InsertChildAt(content,
>-                          mStack[mStackPos - 2].mInsertionPoint++,
>-                          PR_FALSE);
>-  } else {
>-    parent->AppendChildTo(content, PR_FALSE);
>-  }
>+  mStack[mStackPos - 2].Add(content);
> 
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   if (mSink->IsMonolithicContainer(nodeType)) {
>     mSink->mInMonolithicContainer++;
>   }
> 
>   // Special handling for certain tags
>@@ -895,16 +891,29 @@ SinkContext::HaveNotifiedForCurrentConte
> SinkContext::HaveNotifiedForCurrentContent() const
> {
>   if (0 < mStackPos) {
>     nsIContent* parent = mStack[mStackPos - 1].mContent;
>     return mStack[mStackPos-1].mNumFlushed == parent->GetChildCount();
>   }
> 
>   return PR_TRUE;
>+}
>+
>+nsIContent *
>+SinkContext::Node::Add(nsIContent *child)
>+{
>+  if (mInsertionPoint != -1) {
>+    NS_ASSERTION(mNumFlushed == mContent->GetChildCount(),
>+                 "Inserting multiple children without flushing.");
>+    mContent->InsertChildAt(child, mInsertionPoint++, PR_FALSE);
>+  } else {
>+    mContent->AppendChildTo(child, PR_FALSE);
>+  }
>+  return child;
> }
> 
> nsresult
> SinkContext::CloseContainer(const nsHTMLTag aTag, PRBool aMalformed)
> {
>   nsresult result = NS_OK;
> 
>   // Flush any collected text content. Release the last text
>@@ -1148,29 +1157,18 @@ SinkContext::AddLeaf(const nsIParserNode
> 
> nsresult
> SinkContext::AddLeaf(nsGenericHTMLElement* aContent)
> {
>   NS_ASSERTION(mStackPos > 0, "leaf w/o container");
>   if (mStackPos <= 0) {
>     return NS_ERROR_FAILURE;
>   }
>-
>-  nsGenericHTMLElement* parent = mStack[mStackPos - 1].mContent;
>-
>-  // If the parent has an insertion point, insert rather than append.
>-  if (mStack[mStackPos - 1].mInsertionPoint != -1) {
>-    parent->InsertChildAt(aContent,
>-                          mStack[mStackPos - 1].mInsertionPoint++,
>-                          PR_FALSE);
>-  } else {
>-    parent->AppendChildTo(aContent, PR_FALSE);
>-  }
>-
>-  DidAddContent(aContent);
>+  
>+  DidAddContent(mStack[mStackPos - 1].Add(aContent));
> 
> #ifdef DEBUG
>   if (SINK_LOG_TEST(gSinkLogModuleInfo, SINK_ALWAYS_REFLOW)) {
>     mSink->ForceReflow();
>   }
> #endif
> 
>   return NS_OK;
>@@ -1196,35 +1194,26 @@ SinkContext::AddComment(const nsIParserN
> 
>   comment->SetText(aNode.GetText(), PR_FALSE);
> 
>   NS_ASSERTION(mStackPos > 0, "stack out of bounds");
>   if (mStackPos <= 0) {
>     return NS_ERROR_FAILURE;
>   }
> 
>-  nsGenericHTMLElement* parent;
>-  if (!mSink->mBody && !mSink->mFrameset && mSink->mHead) {
>-    // XXXbz but this will make DidAddContent use the wrong parent for
>-    // the notification!  That seems so bogus it's not even funny.
>-    parent = mSink->mHead;
>-  } else {
>-    parent = mStack[mStackPos - 1].mContent;
>+  {
>+    Node &parentNode = mStack[mStackPos - 1];
>+    nsGenericHTMLElement *parent = parentNode.mContent;
>+    if (!mSink->mBody && !mSink->mFrameset && mSink->mHead)
>+      // XXXbz but this will make DidAddContent use the wrong parent for
>+      // the notification!  That seems so bogus it's not even funny.
>+      parentNode.mContent = mSink->mHead;
>+    DidAddContent(parentNode.Add(comment));
>+    parentNode.mContent = parent;
>   }
>-
>-  // If the parent has an insertion point, insert rather than append.
>-  if (mStack[mStackPos - 1].mInsertionPoint != -1) {
>-    parent->InsertChildAt(comment,
>-                          mStack[mStackPos - 1].mInsertionPoint++,
>-                          PR_FALSE);
>-  } else {
>-    parent->AppendChildTo(comment, PR_FALSE);
>-  }
>-
>-  DidAddContent(comment);
> 
> #ifdef DEBUG
>   if (SINK_LOG_TEST(gSinkLogModuleInfo, SINK_ALWAYS_REFLOW)) {
>     mSink->ForceReflow();
>   }
> #endif
> 
>   return rv;
>@@ -1380,20 +1369,21 @@ SinkContext::FlushTags()
>           SINK_TRACE(gSinkLogModuleInfo, SINK_TRACE_REFLOW,
>                      ("SinkContext::FlushTags: tag=%s from newindex=%d at "
>                       "stackPos=%d", tagStr,
>                       mStack[stackPos].mNumFlushed, stackPos));
>         }
> #endif
>         if ((mStack[stackPos].mInsertionPoint != -1) &&
>             (mStackPos > (stackPos + 1))) {
>+          PRInt32 childIndex = mStack[stackPos].mInsertionPoint - 1;
>           nsIContent* child = mStack[stackPos + 1].mContent;
>-          mSink->NotifyInsert(content,
>-                              child,
>-                              mStack[stackPos].mInsertionPoint - 1);
>+          NS_ASSERTION(content->GetChildAt(childIndex) == child,
>+                       "Flushing the wrong child.");
>+          mSink->NotifyInsert(content, child, childIndex);
>         } else {
>           mSink->NotifyAppend(content, mStack[stackPos].mNumFlushed);
>         }
> 
>         flushed = PR_TRUE;
>       }
> 
>       mStack[stackPos].mNumFlushed = childCount;
>@@ -1484,28 +1474,19 @@ SinkContext::FlushText(PRBool* aDidFlush
>       mTextLength = 0;
> 
>       // Add text to its parent
>       NS_ASSERTION(mStackPos > 0, "leaf w/o container");
>       if (mStackPos <= 0) {
>         return NS_ERROR_FAILURE;
>       }
> 
>-      nsGenericHTMLElement* parent = mStack[mStackPos - 1].mContent;
>-      if (mStack[mStackPos - 1].mInsertionPoint != -1) {
>-        parent->InsertChildAt(mLastTextNode,
>-                              mStack[mStackPos - 1].mInsertionPoint++,
>-                              PR_FALSE);
>-      } else {
>-        parent->AppendChildTo(mLastTextNode, PR_FALSE);
>-      }
>+      DidAddContent(mStack[mStackPos - 1].Add(mLastTextNode));
> 
>       didFlush = PR_TRUE;
>-
>-      DidAddContent(mLastTextNode);
>     }
>   }
> 
>   if (aDidFlush) {
>     *aDidFlush = didFlush;
>   }
> 
>   if (aReleaseLast) {
>@@ -1939,18 +1920,20 @@ HTMLContentSink::EndContext(PRInt32 aPos
> 
>   PRInt32 n = mContextStack.Count() - 1;
>   SinkContext* sc = (SinkContext*) mContextStack.ElementAt(n);
> 
>   NS_ASSERTION(sc->mStack[aPosition].mType == mCurrentContext->mStack[0].mType,
>                "ending a wrong context");
> 
>   mCurrentContext->FlushTextAndRelease();
>+  mCurrentContext->FlushTags();
> 
>   sc->mStack[aPosition].mNumFlushed = mCurrentContext->mStack[0].mNumFlushed;
>+  sc->mStack[aPosition].mInsertionPoint = mCurrentContext->mStack[0].mInsertionPoint;
> 
>   for (PRInt32 i = 0; i<mCurrentContext->mStackPos; i++) {
>     NS_IF_RELEASE(mCurrentContext->mStack[i].mContent);
>   }
> 
>   delete [] mCurrentContext->mStack;
> 
>   mCurrentContext->mStack      = nsnull;
>diff --git a/layout/reftests/reftest.list b/layout/reftests/reftest.list
>--- a/layout/reftests/reftest.list
>+++ b/layout/reftests/reftest.list
>@@ -105,8 +105,11 @@ include text-transform/reftest.list
> # xul-document-load/
> include xul-document-load/reftest.list
> 
> # xul grid
> include ../xul/base/src/grid/reftests/reftest.list
> 
> # z-index/
> include z-index/reftest.list
>+
>+# reftest(s) to verify content bugfixes
>+include ../../content/html/document/reftests/reftests.list

Comment 39

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

Attachment: stray Makefile changes removed from previous patch

Please disregard my previous comment.  "Edit attachment as comment" did exactly what it said, if not what I intended...

Comment 40

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

Contexts are basically insertion points. We create them for two reasons:

1. Insert into the head vs. the body. So for example we keep a head context
   around to insert some set of elements in the head always. At this point that
   set might simply be the <title> element.

2. Insert misplaced table stuff before the table. So given the markup
   <table><tr><span><td>, when we hit the <span> we create a new context and tell
   it to insert stuff before the <table> element.

Comment 41

[Boris Zbarsky [:bzbarsky]]

So given that, propagating mInsertionPoint from context to context would be wrong, sounds like.

Ben, I think I've more or less managed to convince myself that we can't insert multiple nodes in a single insertion point without notifying, but I'll need to double-check some of that tomorrow.  I'll also look at that patch first thing in the morning.

Comment 42

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

(In reply to comment #41)
> So given that, propagating mInsertionPoint from context to context would be
> wrong, sounds like.

Yep, and calling FlushTags in EndContext makes that unnecessary anyhow.  This patch is essentially a one-liner (ignoring tests and assertions and the SinkContext::Node::Add refactoring).

Jonas's post suggests that context switching happens relatively infrequently, so there shouldn't be any appreciable performance hit.

Comment 43

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

Attachment: no more back-propagation of mInsertionPoint

The only remaining functional change is the addition of mCurrentContext->FlushTags() to HTMLContentSink::EndContext.  Rest of the patch consists of tests and some refactoring (to use SinkContext::Node::Add).

Comment 44

[Boris Zbarsky [:bzbarsky]]

So.  Let's posit a parent with an insertion point.  We're parsing its kids.  When we OpenContainer a kid or AddLeaf a kid, we stick it into the insertion point.  Then when we call DidAddContent either in the same AddLeaf or when we CloseContainer the kid.  Since we have an insertion point, DidAddContent notifies.  Therefore, we cannot insert multiple kids into the insertion point without notifying on all but maybe the last of them.

Comment 45

[Boris Zbarsky [:bzbarsky]]

OK.  So given that, I'm back to being confused about why this bug happens.  Do we EndContext before we make the DidAddContent call for the insertion point content?

Comment 46

[Boris Zbarsky [:bzbarsky]]

That is, can we assert in EndContext that mNumFlushed == GetChildCount()?  I assume that this bug's testcase would trigger the assert, right?

Comment 47

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

(In reply to comment #45)
> OK.  So given that, I'm back to being confused about why this bug happens.  Do
> we EndContext before we make the DidAddContent call for the insertion point
> content?

Yes, we do.  Think CNavDTD::CloseContainersTo has something to do with that; going to look into it after lunch.

Comment 48

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

Attachment: proposed patch (no longer calling FlushTags)

Summary of changes:

- CNavDTD::HandleSavedTokens is now responsible for instructing
  the content sink to close forms created (e.g., by
  CNavDTD::CreateContextStackFor) during the execution of
  HandleSavedTokens.  Intuitively, CNavDTD::CloseContainersTo
  should do this, but the parser forgets about forms after
  opening them (a deliberate design choice that I didn't make).
  Closing these forms before calling HTMLContentSink::EndContext
  ensures that the sink is completely flushed before it throws
  away the current context (with its mInsertionPoint
  information), which solves the double rendering bug without
  requiring any calls to FlushTags.

- Introduced CNavDTD::CloseResidualStyleTags to fix another bug
  that was involved in the original test case but orthogonal to
  the double rendering issue.  To explain: residual tags get
  pushed inside block elements when the document has any
  malformed tags, and the sink refuses to close a form if any
  tags inside the form are still open.  So form closure fails
  when there are any malformed tags, unless we close all the
  residual tags still open within the form.  This failure causes
  subsequent nodes to nest inside the unclosed form (sometimes
  forms within forms!), as reftest bug448564-1_*.html
  demonstrates.  My bug448564_4*.html reftest verifies that
  CNavDTD::OpenTransientStyles reopens the residual tags.

- Decomposition of SinkContext::Node::Add.  Not a functional
  change, just a little cleanup.

- Miscellaneous assertions.

- Six reftests and one crashtest.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Oh... Right.  Forms.  How does this last patch handle:

<table>
  <form>
    <tr><td><input name="a" value="aval"></td></tr>
    <input type="hidden" name="b" value="bval">
    <input name="c" value="cval">
    <tr><td><input name="d" value="dval" type="submit"></td></tr>
  </form>
</table>

When you submit the form, are all the inputs submitted?  Does anything change if the form is opened before the table open and closed after the table close?  What about:

<table>
  <span><form>
    <tr><td><input name="a" value="aval"></td></tr>
    <input type="hidden" name="b" value="bval">
    <input name="c" value="cval">
    <tr><td><input name="d" value="dval" type="submit"></td></tr>
  </form></span>
</table>

or some such?  In any case, with forms involved like this I can almost see how we could switch contexts while the form is still open.  Given that, the FlushTags is probably what we want.  I just wanted to understand what about this testcase was tickling that situation...

If you do want to make changes to CNavDTD, then mrbkap is your review man for those.  I'm willing to review the sink, but CNavDTD is black magic.  ;)

Comment 50

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

Attachment: new forms tests, back to calling FlushTags for good measure

Blake, can you review the CNavDTD changes in this patch?  Hope my comments in the code make it clear what I was trying to do.

Comment 51

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

(In reply to comment #49)
> When you submit the form, are all the inputs submitted?  Does anything change
> if the form is opened before the table open and closed after the table close?

All the input fields do get submitted, according to the tests I added in the most recent patch.

> In any case, with forms involved like this I can almost see how
> we could switch contexts while the form is still open.  Given that, the
> FlushTags is probably what we want.

I think it was worthwhile to drill a little deeper (i.e., into the parser), but I'm also inclined to keep FlushTags in EndContext.

> If you do want to make changes to CNavDTD, then mrbkap is your review man for
> those.  I'm willing to review the sink, but CNavDTD is black magic.  ;)

Don't blame you :)

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 337795 [details] [diff] [review]
new forms tests, back to calling FlushTags for good measure

The CNavDTD changes look good. bug 136397 tracks the most correct fix for <form> closure (namely, tracking whether we've opened one in CNavDTD, not in the content sink).

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 337795 [details] [diff] [review]
new forms tests, back to calling FlushTags for good measure

sr=bzbarsky, with newlines added to the ends of all those files that have no newlines at the end.  Thanks a ton for digging into this code and getting this sorted out!

Comment 54

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

Attachment: now with newlines

Comment 55

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 39f61d918dae.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Do we need this on any branches, btw?

Comment 57

[Samuel Sidler (old account; do not CC)]

Maybe; nominating for now so we can discuss it on Monday.

Comment 58

[Daniel Veditz [:dveditz]]

Do we need a different 1.9.0.x patch or have things not changed too much?

Comment 59

[Daniel Veditz [:dveditz]]

Ben: please create a FF3.0.x patch (if necessary) and request approval for 1.9.0.5 on it or on this one if it applies. Missed 1.9.0.4, will want this early in the next cycle (for which the tree is only going to be open for about a week).

Comment 60

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

(In reply to comment #59)
> Ben: please create a FF3.0.x patch (if necessary) and request approval for
> 1.9.0.5 on it or on this one if it applies. Missed 1.9.0.4, will want this
> early in the next cycle (for which the tree is only going to be open for about
> a week).

Sure thing.  Should this backpatch include tests, or just the functional changes?  It's just a few lines if tests are not necessary.

Comment 61

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

Tests would be good since we don't want to regress on the 3.0 branch either (actually, almost even more important there).

We have the same test infrastructure running on the branch though so you should be able to use the same tests.

Comment 62

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

(In reply to comment #59)
> Ben: please create a FF3.0.x patch (if necessary) and request approval for
> 1.9.0.5 on it or on this one if it applies. Missed 1.9.0.4, will want this
> early in the next cycle (for which the tree is only going to be open for about
> a week).

Is that the hg tag MOZILLA_1_9_a7_BASE?

Comment 63

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #62)
> Is that the hg tag MOZILLA_1_9_a7_BASE?

Nope, it's CVS HEAD (see https://developer.mozilla.org/en/Mozilla_Source_Code_Via_CVS)

Comment 64

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

Attachment: backport to CVS HEAD

Passes my tests.  Waiting on the tryserver; will update when that's done.

Comment 65

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

(In reply to comment #64)
> Created an attachment (id=345176) [details]
> backport to CVS HEAD
> 
> Passes my tests.  Waiting on the tryserver; will update when that's done.

And it builds on all three platforms: https://build.mozilla.org/tryserver-builds/2008-10-28_15:[e-mail]-1225231280/

Comment 66

[Samuel Sidler (old account; do not CC)]

Comment on attachment 345176 [details] [diff] [review]
backport to CVS HEAD

Applied cleanly and everything. Requesting approval for 1.9.0.5.

Comment 67

[Daniel Veditz [:dveditz]]

Comment on attachment 345176 [details] [diff] [review]
backport to CVS HEAD

approved for 1.9.0.5, a=dveditz for release-drivers

(please wait for tree to open before landing)

Comment 68

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

(In reply to comment #67)
> (From update of attachment 345176 [details] [diff] [review])
> approved for 1.9.0.5, a=dveditz for release-drivers
> 
> (please wait for tree to open before landing)

Since I don't yet have commit access, any ideas whom I should ask to land this?

Comment 69

[Blake Kaplan (:mrbkap) (inactive)]

I'll land this in a couple of hours.

Comment 70

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.9 branch.

Comment 71

[Al Billings [:abillings - ex-MoCo]]

This seems to be working based on the checked in tests. Can we get anyone from Radware to verify the fix?