468 bytes, text/html
15.43 KB, text/plain
5.46 KB, text/plain
32.70 KB, text/html
209 bytes, text/html
252 bytes, text/html
2.24 KB, patch
|Details | Diff | Splinter Review|
See upcoming testcase. It doesn't crash easily, when following the directions in the testcase. But the assertions I get in a debug build with the testcase, I get 100% of the time. It doesn't crash Mozilla1.7, I'm quite certain of that. Also, in Mozilla1.7, the green block doesn't seem 'out of place' (which you can see with the dom inspector).
Created attachment 203780 [details] backtrace from assertions This is a backtrace from the assertions The first assertion, I get while loading the page: ###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlo ck', file c:/mozilla/mozilla/layout/generic/nsBlockReflowState.cpp, line 838 Break: at file c:/mozilla/mozilla/layout/generic/nsBlockReflowState.cpp, line 83 The second I get when hovering over the green bordered box: ###!!! ASSERTION: not in child list: 'nsFrameList(aChildFrame->GetParent()->GetF irstChild(listName)) .ContainsFrame(aChildFrame)', file c:/mozilla/mozilla/layou t/base/nsCSSFrameConstructor.cpp, line 1882 Break: at file c:/mozilla/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 18 82
Created attachment 203781 [details] backtrace from crash This is a backtrace from the crash I once got in a debug build.
Summary: Evil float:right testcase causes assertions and can crash → Evil float:right testcase causes assertions and can crash [@ nsFrame::GetFirstLeaf]
I assume this is the minimal testcase that triggers those asserts, right?
Yes. (the crash is very hard to reproduce)
Created attachment 203844 [details] testcase2 (crashes on hovering) I tried to minimise the testcase from this file. This crashes reliably for me just by hovering over the document.
OK. I'll have to dig into the frame construction here to see why we're screwing up the float containing block... Might not happen till early next week, though.
Maybe useful to know: Regression range between 2005-01-25 and 2005-01-27 from correct display to not showing content at all. Regression range between 2005-03-17 and 2005-03-18 from not showing content at all to showing the green block 'out of place' (when looking with domi). That seems influenced by bug 283385.
Created attachment 203917 [details] Minimized version of testcase 2 Minimized version of "testcase2 (crashes on hovering)". Hopefully useful and not spam...
Thanks for that testcase, José! With that testcase I don't crash in 2005-08-21 trunk build, but I do crash with 2005-08-22 trunk build: 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=2005-08-21+13&maxdate=2005-08-22+08&cvsroot=%2Fcvsroot I also crash with a 2005-09-01 branch build. So maybe a regression from bug 286491 or bug 295767?
Created attachment 203932 [details] Minimal testcase for the assertion This doesn't crash, but I bet fixing it will fix the crashes.
So the issue is that when we're looking for the right "parent frame" in ContentAppended we adjust from the fieldset to the fielset area frame, except if the content is a legend... or rather except if the _first_ content is a legend, for the case here (where ContentAppended appends both the legend and the float at once). So for this case we end up with the fieldset frame as the parent, and that's not a float containing block, so the containing block we find (via GetFloatContainingBlock) for the float is the body. And then we end up getting this assert...
So the real question is how we fix ContentAppended here, basically. Almost seems like we should make it do a bunch of ContentInserted calls for this case. Perhaps we can hack GetInsertionPoint to claim "multiple" here? That's basically what's going on...
Created attachment 203944 [details] [diff] [review] So perhaps something like this
Created attachment 207667 [details] [diff] [review] Alt. fix? Implementing nsFieldSetFrame::GetContentInsertionFrame() seems to fix the crash also. This is just a WIP though, there could be some adjustments needed in nsCSSFrameConstructor with this change... What do you think, is it worth exploring this alternative?
Mats, won't that approach make it impossible to put the legend in the right place?
Comment on attachment 207667 [details] [diff] [review] Alt. fix? Right, don't know what I was thinking... Maybe we should remove the "yet" from the following comment: http://webtools.mozilla.org/bonsai/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1171&root=/cvsroot&mark=8166#8163 because it shouldn't be implemented...
Attachment #207667 - Attachment is obsolete: true
Or at least make it clear that it would have to deal with legends somehow...
Comment on attachment 203944 [details] [diff] [review] So perhaps something like this r+sr=dbaron. Sorry for the delay. I'm not entirely comfortable with this code, though. I filed bug 323926 (feel free to fix!) while looking at GetInsertionPoint. I also wonder whether this really belongs in GetInsertionPoint or in the one caller of GetInsertionPoint that cares about aMultiple, which is ContentAppended, and which does an additional test for childCount>0 (which has a long comment that doesn't make any sense to me) that's equivalent to having multiple insertion points.
I coudn't convince myself of the correctness of putting the check outside this function (since at that point it's non-obvious that we can get to the "right" content node)... I do think we shoud try to make fieldsets non-magical somehow in terms of their frame structure and just remove this code. :(
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 203944 [details] [diff] [review] So perhaps something like this I think we should take this on the branches... This should be pretty safe, and is along the lines of the sort of thing we've been trying to take. Certainly we should take it on the 1.8.1 branch once it's had enough bake time.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: nobody → bzbarsky
Status: REOPENED → NEW
Status: NEW → RESOLVED
Last Resolved: 13 years ago → 13 years ago
Resolution: --- → FIXED
Verified FIXED using: Testcase 1: https://bugzilla.mozilla.org/attachment.cgi?id=203779 Testcase 2: https://bugzilla.mozilla.org/attachment.cgi?id=203844 Testcase 3: https://bugzilla.mozilla.org/attachment.cgi?id=203917 on both Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060119 Firefox/1.6a1 and SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060119 Mozilla/1.0 (both trunk, of course)
Status: RESOLVED → VERIFIED
Attachment #203944 - Flags: approval1.8.1? → branch-1.8.1?(dbaron)
13 years ago
Attachment #203944 - Flags: branch-1.8.1?(dbaron) → branch-1.8.1+
Fixed on 1.8.1 branch.
Comment on attachment 203944 [details] [diff] [review] So perhaps something like this approved for 1.8.0 branch, a=dveditz
Attachment #203944 - Flags: approval184.108.40.206? → approval220.127.116.11+
Fixed 18.104.22.168 branch
Marking [rft-dl] (ready for testing in Firefox 22.214.171.124 release candidates)
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:126.96.36.199) Gecko/20060301 Firefox/188.8.131.52, no crash with any of the testcases.
Keywords: fixed184.108.40.206 → verified220.127.116.11
Crash Signature: [@ nsFrame::GetFirstLeaf]
You need to log in before you can comment on or make changes to this bug.