remove crash prone form hack from frame construction

VERIFIED FIXED in mozilla1.8.1beta2

Status

()

Core
Layout
--
critical
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: rstrong, Assigned: Bernd)

Tracking

(4 keywords)

Trunk
mozilla1.8.1beta2
x86
Windows XP
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 1 obsolete attachment)

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:
Created attachment 177122 [details]
testcase

The testcase contains
<BODY STYLE="display:table-row;">
<SPAN>
1
</SPAN>
<FORM>
<DIV STYLE="float:left;">
1
<FONT>
1
</FONT>
</DIV>
</FORM>
</BODY>
Keywords: crash, testcase
(Assignee)

Comment 4

13 years ago
Created attachment 177167 [details] [diff] [review]
patch

Boris could you explain why we need that hack in the first place?
(Assignee)

Comment 5

13 years ago
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
(Assignee)

Comment 6

13 years ago
see bug 41806 why we have the margin at all.
Assignee: nobody → bernd_mozilla
(Assignee)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
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?
(Assignee)

Comment 9

13 years ago
ehm I did mean bug 41806?
This is now WFM - perhaps the checkin for bug 263825 has fixed this?
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 11

13 years ago
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
(Assignee)

Updated

13 years ago
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

Updated

12 years ago
Blocks: 330998
(Assignee)

Comment 13

12 years ago
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.  :(
(Assignee)

Comment 15

12 years ago
Created attachment 219504 [details] [diff] [review]
revised patch
Attachment #177167 - Attachment is obsolete: true
Attachment #219504 - Flags: superreview?(bzbarsky)
Attachment #219504 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

12 years ago
Created attachment 219505 [details]
quirksmode
(Assignee)

Comment 17

12 years ago
Created attachment 219506 [details]
almoststandard
(Assignee)

Comment 18

12 years ago
Created attachment 219507 [details]
strict
(Assignee)

Comment 19

12 years ago
Created attachment 219508 [details]
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.
(Assignee)

Comment 25

12 years ago
Created attachment 221923 [details] [diff] [review]
revised patch

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

Comment 27

12 years ago
Created attachment 221962 [details] [diff] [review]
patch that got checked in

Boris, if you could verify that I followed your wishes...
(Assignee)

Comment 28

12 years ago
fix checked in, I will wait some time till I request the branch approvals
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago12 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?
(Assignee)

Comment 31

12 years ago
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?
(Assignee)

Comment 36

12 years ago
Created attachment 231416 [details] [diff] [review]
patch against MOZILLA_1_8_BRANCH
(Assignee)

Comment 37

12 years ago
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+
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...
(Assignee)

Comment 42

12 years ago
fixed on 1.8.0
Keywords: fixed1.8.0.7
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
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1

Comment 44

11 years ago
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.