Closed Bug 285727 Opened 20 years ago Closed 19 years ago

remove crash prone form hack from frame construction

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8.1beta2

People

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

References

Details

(4 keywords)

Attachments

(11 files, 1 obsolete file)

testcase
139 bytes, text/html
Details
assertions
712 bytes, text/plain
Details
stack trace
8.96 KB, text/plain
Details
revised patch
4.24 KB, patch
bzbarsky
: review-
bzbarsky
: superreview-
Details | Diff | Splinter Review
quirksmode
1.21 KB, text/html
Details
almoststandard
1.31 KB, text/html
Details
strict
1.26 KB, text/html
Details
xhtml
1.39 KB, application/xhtml+xml
Details
revised patch
11.50 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
patch that got checked in
10.03 KB, patch
bzbarsky
: approval-branch-1.8.1+
Details | Diff | Splinter Review
patch against MOZILLA_1_8_BRANCH
9.67 KB, patch
dveditz
: approval1.8.0.7+
Details | Diff | Splinter Review
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: 20 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 patch — — Splinter 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 patch — — Splinter 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: 20 years ago → 19 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
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
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.

Attachment

General

Creator:
Created:
Updated:
Size: