Closed Bug 285727 Opened 16 years ago Closed 15 years ago

remove crash prone form hack from frame construction

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.8.1beta2

People

(Reporter: robert.strong.bugs, Assigned: bernd_mozilla)

References

Details

(4 keywords)

Attachments

(11 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050310 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050310 Firefox/1.0+

The soon to be attached simplified testcase causes a crash @
nsContainerFrame::GetFrameForPointUsing.

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB4267990H

Reproducible: Always

Steps to Reproduce:
Attached file testcase
The testcase contains
<BODY STYLE="display:table-row;">
<SPAN>
1
</SPAN>
<FORM>
<DIV STYLE="float:left;">
1
<FONT>
1
</FONT>
</DIV>
</FORM>
</BODY>
Attached patch patch (obsolete) — Splinter Review
Boris could you explain why we need that hack in the first place?
bernd	bz: if you could comment on the form pseudo evil hack over the weekend
(bug 285727) I dont get why we have that at all
	bz	bernd: we can't remove that hack
	bz	bernd: we need to find another way....
	bz	bernd: Though we _could_ probably replace that code with some quirks-only CSS...
	bernd	bz: I dont get what that hack is trying to accomplish
	bz	bernd: consider the following HTML
	bz	<table><form><tr><td>Text</td></tr></form></table>
	bernd	bz: doesnt the parser push the form outside
	bz	bernd: our parser produces the following DOM:
<table><form/><tr><td>Text</td></tr></table>
	bz	bernd: But in quirks mode (used to be standards too, but that was removed
recently), <form> has a bottom margin
	bz	bernd: so if we create a frame for it, there will be this random gap
	bz	bernd: In standards mode, that's not an issue, but creating a pseudo-row for
the form will still screw up rowspanning
	bernd	bz: but the form is display: block so why should I not wrap it
	bz	bernd: so, somehow, what we want is for HTML only (not XHTML) to make <form>
tags which are empty and whose parent is a <table>/<tbody>/<thead>/<tfoot>/<tr>
display: none
	bz	bernd: not only do we want to not wrap it, we want to not create a frame for it
	bernd	bz: aaahhhh
	bz	bernd: note that's what we do in TableProcessChild
	bernd	but we doo
	bz	bernd: yes, apparently so. My fault there... :(
	bz	bernd: so what we should _really_ do for the form cases in AdjustParentFrame
is somehow indicate to the caller that we want to just bail out....
	bernd	bz: so we should not create a frame for the form but we should create the
form children?
	bz	bernd: alternately, we should remove all this code and do what I said --
compute display to "none" on forms like this.
	bz	bernd: form has no children in thie case..
	bz	bernd: er, this case.
	bernd	bz: it has
	bz	bernd: not in the DOM
	bz	bernd: oh, in this case. Sure.
	bz	bernd: In this case we _do_ want to wrap it.
	bz	bernd: basically, we want the following CSS to apply:
	bz	table > form:empty, tr > form:empty, tbody > form:empty, thead > form:empty,
tfoot > form:empty { display: none ! important }
	bz	But only for HTML documents.
	bz	Not for XHTML
	bz	(as determined by the MIME type and all that)
	bz	bernd: and then we want to remove all this frame constructor code
see bug 41806 why we have the margin at all.
Assignee: nobody → bernd_mozilla
in order to avoid the form margin people adopted the technology of placing the
form inside the table tags but not inside the table cell. We need to preserve
this hack.
Anne, when you worked on the patch for bug did you ever see the case when a form
as a direct child of a table had that 1em margin?
ehm I did mean bug 41806?
This is now WFM - perhaps the checkin for bug 263825 has fixed this?
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
the crash might be fixed, but this bug/patch is about preventing to enter the
condition at all 
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: Crash [@ nsContainerFrame::GetFrameForPointUsing ] → remove form hack from frame construction
Summary: remove form hack from frame construction → remove crash prone form hack from frame construction
Attachment #177122 - Attachment description: testcase (causes crash) → testcase
good enough. I've removed the crash keyword and removed crash from the name of
the testcase to avoid further WFM comments in the future.
Keywords: crash
Blocks: 330998
Boris, 

I would like to go ahead on this issue to fix bug 330998. If you could hit me a little bit with a clue-stick

>bz	But only for HTML documents.
>bz	Not for XHTML
>bz	(as determined by the MIME type and all that)

If we add the rules to quirk.css only isn't that enough?
I think we need this "quirk" for standards-mode HTML too.  :(
Attached patch revised patchSplinter Review
Attachment #177167 - Attachment is obsolete: true
Attachment #219504 - Flags: superreview?(bzbarsky)
Attachment #219504 - Flags: review?(bzbarsky)
Attached file quirksmode
Attached file almoststandard
Attached file strict
Attached file xhtml
please note that html.css does not influence the xhtml case as it did not have the margin before.
Comment on attachment 219504 [details] [diff] [review]
revised patch

It influences the XHTML case if the page has styled the form (say given it a border), no?
So maybe what we should do is add a rule to nsHTMLStyleSheet to do this for empty forms in table tags?  That would make it have lower specificity than page-specified styles, but maybe that's ok?
Comment on attachment 219504 [details] [diff] [review]
revised patch

Per comment 20.
Attachment #219504 - Flags: superreview?(bzbarsky)
Attachment #219504 - Flags: superreview-
Attachment #219504 - Flags: review?(bzbarsky)
Attachment #219504 - Flags: review-
So I'm not sure what the question to me is:  is the question whether the html.css changes in attachment 219504 [details] [diff] [review] should go in nsHTMLStyleSheet instead?

If the reason you want to put the equivalent in nsHTMLStyleSheet is that you don't want it to apply to XHTML, then that is fine with me, although maybe we should have a way of doing that in a style sheet...
Yes, the idea is to not apply to XHTML.
Attached patch revised patchSplinter Review
I merely do understand what I am doing here, so do not claim that this is the right thing to do. One problem with this patch is that it copies code from http://lxr.mozilla.org/seamonkey/source/layout/style/nsCSSRuleProcessor.cpp#922 I don't know where to place this function correctly.

Boris could you look at it and give some guidance, thanks.
Attachment #221923 - Flags: superreview?(bzbarsky)
Attachment #221923 - Flags: review?(bzbarsky)
Comment on attachment 221923 [details] [diff] [review]
revised patch

>Index: style/nsHTMLStyleSheet.cpp
>+static PRBool IsSignificantChild(nsIContent* aChild, 

I don't think you need this.  More below.

>+      else if (tag == nsHTMLAtoms::form) {

So two thing I would like different here:

1)  We don't want this to apply for XHTML, so we should check that.  Simplest way is probably to  content->GetOwnerDoc() and then ask that whether it's case-sensitive.

2)  No need for all this whitespace-ignoring stuff.  The <form>s that the parser generates are really truly empty.  Just check whether content->GetChildCount() is 0 and be done with it.

>+        nsIContent* parent = content->GetParent();
>+        if (parent) {
>+          nsIAtom* parentTag = parent->Tag();
>+          if ((nsHTMLAtoms::table == parentTag) ||

So... you probably want to make sure that parent is also HTML or something along those lines.  Note that we used to have an IsContentOfType() check on the parent in the frame constructor.

>Index: style/nsHTMLStyleSheet.h
>+    #ifdef DEBUG
>+    NS_IMETHOD List(FILE* out = stdout, PRInt32 aIndent = 0) const;
>+  #endif

Fix the #ifdef indent?

Looks good otherwise.
Attachment #221923 - Flags: superreview?(bzbarsky)
Attachment #221923 - Flags: superreview+
Attachment #221923 - Flags: review?(bzbarsky)
Attachment #221923 - Flags: review+
Boris, if you could verify that I followed your wishes...
fix checked in, I will wait some time till I request the branch approvals
Status: REOPENED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
That looks good, but you don't need the:

> +#include "nsITextContent.h"

thing, do you?
Actually, one more thing... We should probably only set mDisplay if it's null when we get to it here, no?
Comment on attachment 221962 [details] [diff] [review]
patch that got checked in

Boris can this as it is go onto 1.8.1?
Attachment #221962 - Flags: approval-branch-1.8.1?(bzbarsky)
Comment on attachment 221962 [details] [diff] [review]
patch that got checked in

This is ok, if the issues in comment 29 and comment 30 get resolved....  They need to be resolved on trunk too, no?
Attachment #221962 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Approved 1.8.1 but not checked in, nominating for visibility because I think we do still want this crash fix. Probably not needed on 1.8.0, doesn't seem to be a topcrash or exploitable.
Flags: blocking1.8.1?
Keywords: crash
Flags: blocking1.8.1? → blocking1.8.1+
Target Milestone: --- → mozilla1.8.1beta2
might still want it on 1.8.0 just as a stability fix, would unblock any testing bug 330998 is blocking.
Flags: blocking1.8.0.6?
Any chance of getting a 1.8.1-ready patch here in the next few days?
fixed in 1.8.1
Keywords: fixed1.8.1
passing on this one for 1.8.0.x, not a security issue.
Flags: blocking1.8.0.7? → blocking1.8.0.7-
For what it's worth, this is an easy source of deleted-frame crashes.  So it _is_ in fact a security issue.
Flags: blocking1.8.0.7- → blocking1.8.0.7?
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Comment on attachment 231416 [details] [diff] [review]
patch against MOZILLA_1_8_BRANCH

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #231416 - Flags: approval1.8.0.7+
Depends on: 348455
So wait.  What happened to comment 30, exactly?  It never got addressed, as far as I can tell.

Bug 348455 filed to make sure that happens...
fixed on 1.8.0
Keywords: fixed1.8.0.7
Depends on: 349695
Verified fixed on 1.8.0.7 branch and on 1.8.1 branch by testing the testcase frombug 330998, which got fixed by the patch in this bug.
Status: RESOLVED → VERIFIED
I checked in Rob Strong's original testcase as a crashtest.

Bernd, do you want to do anything with the other testcases you made?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.