Bug #256624 “[CVE-2008-3444] Firefox 3.0.1 crash via a crafted b...” : Bugs : firefox-3.0 package : Ubuntu

Revision history for this message

In Mozilla Bugzilla #448564, Jst (jst) wrote on 2008-07-30:

#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.

Revision history for this message

In Mozilla Bugzilla #448564, Martijn-martijn (martijn-martijn) wrote on 2008-07-30:

#2

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.

Revision history for this message

In Mozilla Bugzilla #448564, Martijn-martijn (martijn-martijn) wrote on 2008-07-30:

#3

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

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-07-30:

#4

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.

Revision history for this message

In Mozilla Bugzilla #448564, Jruderman (jruderman) wrote on 2008-08-03:

#5

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

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-04:

#6

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.

Revision history for this message

In Mozilla Bugzilla #448564, Jonas-sicking (jonas-sicking) wrote on 2008-08-04:

#7

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.

Revision history for this message

In Mozilla Bugzilla #448564, Jst (jst) wrote on 2008-08-04:

#8

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...

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-04:

#9

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-04:

#10

(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.

Revision history for this message

In Mozilla Bugzilla #448564, Jonas-sicking (jonas-sicking) wrote on 2008-08-04:

#11

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 :)

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-04:

#12

(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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-06:

#13

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.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-06:

#14

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|

Revision history for this message

In Mozilla Bugzilla #448564, Dveditz (dveditz) wrote on 2008-08-06:

#15

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

Revision history for this message

Till Ulen (tillulen) wrote on 2008-08-10:

#16

Binary package hint: firefox-3.0

CVE-2008-3444 description:

"The content layout component in Mozilla Firefox 3.0 and 3.0.1 allows remote attackers to cause a denial of service (NULL pointer dereference and application crash) via a crafted but well-formed web page that contains "a simple set of legitimate HTML tags." "

http://nvd.nist.gov/nvd.cfm?cvename=CVE-2008-3444

More information:
http://blog.mozilla.com/security/2008/07/30/low-risk-denial-of-service-in-firefox/
https://bugzilla.mozilla.org/show_bug.cgi?id=448564 (private bug)

Revision history for this message

Till Ulen (tillulen) wrote on 2008-08-10:

#17

Adding CVE reference: CVE-2008-3444

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-14:

#18

Created attachment 333828
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.

Revision history for this message

In Mozilla Bugzilla #448564, Mrbkap (mrbkap) wrote on 2008-08-14:

#19

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

Revision history for this message

In Mozilla Bugzilla #448564, Jonas-sicking (jonas-sicking) wrote on 2008-08-14:

#20

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-14:

#21

(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!

Revision history for this message

In Mozilla Bugzilla #448564, Jonas-sicking (jonas-sicking) wrote on 2008-08-14:

#22

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 :)

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-19:

#23

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....

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-19:

#24

I'll try to get to this review tomorrow.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-26:

#25

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?

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-26:

#26

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-27:

#27

(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.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-27:

#28

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-27:

#29

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-27:

#30

(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.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-27:

#31

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-27:

#32

Created attachment 335807
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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#33

Created attachment 335828
same patch, spiffy new reftests

2 reftests, 1 crashtest

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#34

(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/

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-28:

#35

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... ;)

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#36

(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).

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#37

(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...

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#38

Created attachment 335944
decomposed SinkContext::Node::Add

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

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#39

Created attachment 335973
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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#40

Download full text (12.6 KiB)

Comment on attachment 335973
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/...

Comment on attachment 335973
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

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-28:

#41

Created attachment 335976
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...

Revision history for this message

In Mozilla Bugzilla #448564, Jonas-sicking (jonas-sicking) wrote on 2008-08-28:

#42

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.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-29:

#43

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-29:

#44

(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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-29:

#45

Created attachment 335997
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).

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-29:

#46

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.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-29:

#47

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?

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-08-29:

#48

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

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-08-29:

#49

(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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-09-06:

#50

Created attachment 337156
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.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-09-06:

#51

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

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:

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. ;)

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-09-10:

#52

Created attachment 337795
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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-09-10:

#53

(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 :)

Revision history for this message

In Mozilla Bugzilla #448564, Mrbkap (mrbkap) wrote on 2008-09-11:

#54

Comment on attachment 337795
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).

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-09-26:

#55

Comment on attachment 337795
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!

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-09-26:

#56

Created attachment 340626
now with newlines

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-10-12:

#57

Pushed changeset 39f61d918dae.

Revision history for this message

In Mozilla Bugzilla #448564, Bzbarsky (bzbarsky) wrote on 2008-10-12:

#58

Do we need this on any branches, btw?

Revision history for this message

In Mozilla Bugzilla #448564, Samuel-sidler+old (samuel-sidler+old) wrote on 2008-10-12:

#59

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

Revision history for this message

In Mozilla Bugzilla #448564, Dveditz (dveditz) wrote on 2008-10-15:

#60

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

Revision history for this message

In Mozilla Bugzilla #448564, Dveditz (dveditz) wrote on 2008-10-22:

#61

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).

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-10-22:

#62

(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.

Revision history for this message

In Mozilla Bugzilla #448564, Jonas-sicking (jonas-sicking) wrote on 2008-10-23:

#63

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.

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-10-23:

#64

(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?

Revision history for this message

In Mozilla Bugzilla #448564, Gavin Sharp (gavin-sharp) wrote on 2008-10-23:

#65

(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)

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-10-28:

#66

Created attachment 345176
backport to CVS HEAD

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

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-10-29:

#67

(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:<email address hidden>/

Revision history for this message

In Mozilla Bugzilla #448564, Samuel-sidler+old (samuel-sidler+old) wrote on 2008-10-29:

#68

Comment on attachment 345176
backport to CVS HEAD

Applied cleanly and everything. Requesting approval for 1.9.0.5.

Revision history for this message

In Mozilla Bugzilla #448564, Dveditz (dveditz) wrote on 2008-11-03:

#69

Comment on attachment 345176
backport to CVS HEAD

approved for 1.9.0.5, a=dveditz for release-drivers

(please wait for tree to open before landing)

Revision history for this message

In Mozilla Bugzilla #448564, Mozilla+ben (mozilla+ben) wrote on 2008-11-17:

#70

(In reply to comment #67)
> (From update of attachment 345176 [details])
> 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?

Revision history for this message

In Mozilla Bugzilla #448564, Mrbkap (mrbkap) wrote on 2008-11-18:

#71

I'll land this in a couple of hours.

Revision history for this message

In Mozilla Bugzilla #448564, Mrbkap (mrbkap) wrote on 2008-11-18:

#72

Fixed on the 1.9 branch.

Revision history for this message

In Mozilla Bugzilla #448564, Abillings (abillings) wrote on 2008-11-26:

#73

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

Bug Watch Updater (bug-watch-updater) on 2008-12-17

Changed in firefox:
status:	Unknown → Fix Released

Revision history for this message

Martin Mai (mrkanister-deactivatedaccount-deactivatedaccount) wrote on 2009-01-22:

#74

Fixed upstream.

Changed in firefox-3.0:
status:	New → Fix Committed

Revision history for this message

Micah Gersten (micahg) wrote on 2009-07-23:

#75

This was released in FF 3.0.5.

Changed in firefox-3.0 (Ubuntu):
status:	Fix Committed → Fix Released

Bug Watch Updater (bug-watch-updater) on 2010-09-18

Changed in firefox:
importance:	Unknown → Medium

Affects		Status	Importance	Assigned to	Milestone
	Mozilla Firefox	Fix Released	Medium	mozilla-bugs #448564
	firefox-3.0 (Ubuntu)	Fix Released	Undecided	Unassigned

Ubuntu
firefox-3.0 package

[CVE-2008-3444] Firefox 3.0.1 crash via a crafted but well-formed web page

Bug Description

CVE References

Other bug subscribers

Remote bug watches

Ubuntufirefox-3.0 package

[CVE-2008-3444] Firefox 3.0.1 crash via a crafted but well-formed web page

Bug Description

CVE References

Other bug subscribers

Remote bug watches

Ubuntu
firefox-3.0 package