Closed
Bug 90205
Opened 23 years ago
Closed 23 years ago
{ib} Most links on this site crash Mozilla
Categories
(Core :: Layout, defect, P2)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla0.9.3
People
(Reporter: bugzilla, Assigned: waterson)
References
()
Details
(Keywords: crash, testcase, topembed)
Attachments
(5 files)
236 bytes,
text/html
|
Details | |
1.02 KB,
patch
|
Details | Diff | Splinter Review | |
5.26 KB,
patch
|
Details | Diff | Splinter Review | |
6.31 KB,
patch
|
Details | Diff | Splinter Review | |
2.04 KB,
patch
|
Details | Diff | Splinter Review |
Build ID: 2001071008
Updated•23 years ago
|
Comment 1•23 years ago
|
||
Sam, if you are crashing in Mozilla the best thing you can do to help the developers fix your bug is to attach a stacktrace. If you're not building yourself you are not out of luck. Mozilla (thanks to a very cool donation from Netscape) releases nightly and milestone builds with Full Circle's Talkback. Talkback should catch most crashes and offer to send in a crash report. I can retrieve that crash report and attach it to your bug report if you provide either the Incident ID (you can get it by running the talkback program from /components/talkback/) or you can let me know the email address you used to submit the report and the time of sending. Thanks for your help in testing Mozilla and reporting bugs.
Comment 2•23 years ago
|
||
unable to reproduce any crashes clicking about 30 random links at this site with 071004/8 win32 and linux mozilla trunk builds. Sam, can you please take a look at the bug writing guidelines. without specific steps it is very difficult to reproduce your problems and get any traction on this bug. http://www.mozilla.org/quality/bug-writing-guidelines.html Thanks.
Which builds use talkback? I currently do: wget http://ftp.mozilla.org/pub/mozilla/nightly/latest/mozilla-i686-pc-linux-gnu.tar.gz I can recreate this bug on two machines (both running Linux) by clicking on any of the stuff under "July Promotion" which include "Netblaster II Hub Save $50", "NetblasterII PCI Card Save $20" etc... the images there... http://store.yahoo.com/sohowaredirect/ncp600.html Sorry about the poor bug report.
http://store.yahoo.com/sohowaredirect/ncp600.html is a crasher...
Comment 5•23 years ago
|
||
Incident ID 32746493 Stack Signature 0x00000000 0377814f Bug ID Trigger Time 2001-07-10 15:02:45 User Comments bug 90205 Build ID 2001071006 Product ID MozillaTrunk Platform ID LinuxIntel Stack Trace 0x00000000 nsCOMPtr_base::assign_from_helper() nsCSSFrameConstructor::StyleChangeReflow() nsCSSFrameConstructor::ProcessRestyledFrames() nsCSSFrameConstructor::AttributeChanged() StyleSetImpl::AttributeChanged() PresShell::AttributeChanged() nsDocument::AttributeChanged() nsHTMLDocument::AttributeChanged() nsGenericHTMLElement::SetAttribute() HTMLContentSink::AddAttributes() HTMLContentSink::OpenBody() CNavDTD::OpenBody() CNavDTD::OpenContainer() CNavDTD::HandleStartToken() CNavDTD::HandleToken() CNavDTD::BuildModel() nsParser::BuildModel() nsParser::ResumeParse() nsParser::ContinueParsing() CSSLoaderImpl::Cleanup() CSSLoaderImpl::SheetComplete() CSSLoaderImpl::ParseSheet() CSSLoaderImpl::DidLoadStyle() SheetLoadData::OnStreamComplete() nsStreamLoader::OnStopRequest() nsHttpChannel::OnStopRequest() nsOnStopRequestEvent::HandleEvent() nsARequestObserverEvent::HandlePLEvent() PL_HandleEvent() PL_ProcessPendingEvents() nsEventQueueImpl::ProcessPendingEvents() event_processor_callback() our_gdk_io_invoke() libglib-1.2.so.0 + 0xeaca (0x4034baca) libglib-1.2.so.0 + 0x10186 (0x4034d186) libglib-1.2.so.0 + 0x10751 (0x4034d751) libglib-1.2.so.0 + 0x108f1 (0x4034d8f1) libgtk-1.2.so.0 + 0x8c8e9 (0x402718e9) nsAppShell::Run() nsAppShellService::Run() main1() main() libc.so.6 + 0x189cb (0x404469cb)
Comment 6•23 years ago
|
||
over to stylesystem.
Assignee: asa → pierre
Status: UNCONFIRMED → NEW
Component: Browser-General → Style System
Ever confirmed: true
QA Contact: doronr → ian
Comment 7•23 years ago
|
||
I confirm the problem on the Mac too. It crashes in nsCSSFrameConstructor::StyleChangeReflow() when calling nsCOMPtr<nsIBox> box = do_QueryInterface(aFrame, &rv) because aFrame is bad. Reassigned to Layout/waterson. It could be related to bug 58674.
Assignee: pierre → waterson
Component: Style System → Layout
OS: Linux → All
Hardware: PC → All
Assignee | ||
Comment 8•23 years ago
|
||
Hmm, I'm not able to reproduce on Win32 with a build from today: I tried banging around the site for a while. Is this still a problem?
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.3
Assignee | ||
Comment 9•23 years ago
|
||
...yup. The above link crashes me on both Mac and Linux, but not Win32.
Assignee | ||
Comment 10•23 years ago
|
||
Assignee | ||
Comment 11•23 years ago
|
||
It turns out that this page has an _extra_ <BODY> tag right at the end! This (plus some explicit font-face style, a floats, and an {ib}) are causing the reframe code to get its knickers in a knot somehow. I really haven't figured it all out yet, but have noticed a couple weird things along the way that I thought I'd note: 1. It seems like some of the logic in nsRuleNode::ComputeFontData() may be a bit squirrely. With the test case that I've attached above, I'm seeing nsFont's get created who's mFont.name is ``serif,serif''. Perhaps we need something like this? (Sorry for the wrapped patch.) Index: nsRuleNode.cpp =================================================================== RCS file: /cvsroot/mozilla/content/html/style/src/nsRuleNode.cpp,v retrieving revision 3.6 diff -u -r3.6 nsRuleNode.cpp --- nsRuleNode.cpp 2001/06/29 22:42:41 3.6 +++ nsRuleNode.cpp 2001/07/18 05:23:24 @@ -1176,8 +1176,14 @@ // MJA: bug 31816 PRBool isMozFixed = font->mFont.name.EqualsIgnoreCase("-moz-fixed"); if (chromeOverride || useDocumentFonts) { - font->mFont.name += nsAutoString(NS_LITERAL_STRING(",")) + defaultFont.name; - font->mFixedFont.name += nsAutoString(NS_LITERAL_STRING(",")) + defaultFixedFont.name; + if (font->mFont.name != defaultFont.name) { + font->mFont.name += + NS_LITERAL_STRING(",") + defaultFont.name; + } + if (font->mFixedFont.name != defaultFixedFont.name) { + font->mFixedFont.name += + NS_LITERAL_STRING(",") + defaultFixedFont.name; + } } else { // now set to defaults In other words, when constructing the new font's name, we don't need to specify the default as an alternative if the new font's name is the same as the default font, right? (dbaron, might this have anything to do with the preponderance of font cache misses you were seeing?) 2. In nsStyleFont::CalcDifference(), we set the hint to REFLOW if the flags are different. This seems a bit heavy-handed, because it seems like we'd only need to reflow if the flags we're different _and_ the resolved fonts that resulted from these flags being different were different. For what it's worth, NS_STYLE_FONT_[FACE|SIZE]_EXPLICIT are write-only. No code ever cares that this flag is set. (Well, okay, some #if 0'd form control code reads it.) I assume that this flag means that a specific font face or size was requested: why do we care? Anyway, if I stuff the flag (hand twiddled it the debugger), the crash goes away because we stop adding frames beneath the body's frame to the change list. Which brings me to what appears to be the real cause of the crash: we have a list of frames that we need to process as the result of a restyle. Specifically, nsCSSFrameConstructor::AttributeChanged() builds up this list by calling nsFrameManager::ComputeStyleChangeFor(), which recursively adds child frames. Then we process the frames. Processing the first frame in the list (the parent) destroys frames that appear later in the list (the children), so we crash. Anyway, seems like there may be several bugs here. I'm still digging.
Assignee | ||
Comment 12•23 years ago
|
||
Okay, I see what's causing the crash. We have a content model like this: <body> <span style="font-face: serif;"> <span style="float: left;"></span> </span> <font> <form></form> </font> <body> </body> Since the <form> is "display: block" inside the inline <font>, we end up with a frame model like this: block(body)< inline(span)< placeholder(span)< > > inline(font)< > block(font)< block(form)< > > > So far, there's one thing wrong with this picture (well, maybe not wrong -- but weird). The second <body> element didn't get its own frame, so any changes that get made to that element are affecting the main body frame! And such a change does arrive, the TOPMARGIN attribute is set. This causes an attribute change notification on the body. We detect a style hint of CONTENT for that frame, and begin to process children. Unfortunately, since we've explicitly selected a font-face on one of the spans, CalcDifference() promotes the contentual change to a REFLOW change, and that propagates to the remainder of the frames. Now we begin to process the restyled frames. We go from back-to-front, so the first frame we'll process is the <font> element's frame -- er, the _second_ of the <font> element's frames, which is the anonymous block that was created to parent the block that appeared in the inline. Since that frame is special, the {ib} code punts and asks to ReframeContainingBlock(); that is, it recreates the frame hierarchy from the <body> frame down. Now we process the next frame in the frame change list, which is the inline <font> frame. Unfortunately, this frame was destroyed in the fire, so we're out of luck and crash. To fix this properly, I think that we'll need to teach ReResolveStyleContext() about {ib}, just like it has to know about continuing frames. We should probably open other bugs on the overzealous promotion of font style changes; I'll do that once I understand it better.
Assignee | ||
Comment 13•23 years ago
|
||
...or, maybe we should just not reconstruct the frame tree if all that's being requested here is a reflow.
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
``Dammit Jim, I'm a reflow, not a doctor!'' There should be no reason to reconstruct the frame tree for a style-change reflow. If {ib}'s can handle resize reflows, they can handle style change reflows, too.
Assignee | ||
Comment 16•23 years ago
|
||
Assignee | ||
Comment 17•23 years ago
|
||
After thinking about this a bit more, I think that we actually need to retarget the style-change reflow to the first ``normal'' frame that parents the hierarchy, at a minimum. If an inline frame is split and requires a style-change reflow, then we'll want to make sure that we reflow the ``special'' anonymous {ib} siblings of the target frame, too. The only way to be sure that this will happen is to target the style-change reflow to the block at the top of the hierarchy (a style-change marks all the lines in the block dirty). The above patch is a bit more conservative, and retargets the reflow to the floater containing block of the first ``normal'' parent.
Comment 18•23 years ago
|
||
It seems like the name GetFloaterContainingBlock is now a little bit misleading (since it is not just for floaters?), but other than that the change makes sense and looks good. [s]r=attinasi
Comment 19•23 years ago
|
||
what are the odds that this can get on the branch? the patch might look a little smaller without the large comments and if (foo) --> if (foo == PR_TRUE) ;)
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
Assignee | ||
Comment 22•23 years ago
|
||
Per blake's prompting, I've created a more modest version of the patch (fewer lines! less scary!) for consideration on the branch if we get to spin a limbo candidate, attachment 42871 [details] [diff] [review].
Keywords: nsBranch
Assignee | ||
Comment 23•23 years ago
|
||
Here's what attachment 42870 [details] [diff] [review] is all about: 1. I changed my mind and decided to let the reflow be targeted at the first non-IB block frame. If it happens to be floated, so be it: things will work out fine. 2. I prettied up the logic (IMO) in ReframeContainingBlock(). Should save a handful of instructions, anyway. 3. nsCOMPtr and Pascal-ism cleanup. Attachment 42871 [details] [diff], which is intended for the branch, only has (1) included in it.
Comment 24•23 years ago
|
||
r=karnaze
Comment 25•23 years ago
|
||
>Additional Comments From Chris Waterson 2001-07-17 22:41 >1. It seems like some of the logic in nsRuleNode::ComputeFontData() may be a >bit squirrely. With the test case that I've attached above, I'm seeing nsFont's >get created who's mFont.name is ``serif,serif''. Perhaps we need something like >this? Interesting that you came across this. It it one of those little details that need close scrutiny for appreciation. I handled it in a different way in FONT_20011307_BRANCH as follows: +static void +SetFont(..., PRBool aIsGeneric, ...) [...] + // MJA: bug 31816 + if (aChromeOverride || aUseDocumentFonts) { + if (!aIsGeneric) { + // only bother appending fallback fonts if this isn't a fallback generic font itself + aFont->mFont.name.Append((PRUnichar)','); + aFont->mFont.name.Append(aDefaultFont.name); } [...] } aIsGeneric is a boolean set earlier if the name was found to be generic (this way, it saves one strcmp since I needed to test for generic anyway).
Assignee | ||
Comment 26•23 years ago
|
||
rbs: that looks good. N.B. that I've checked in the font flags changes from bug 91478, so the original test case may not cause a crash anymore.
Comment 27•23 years ago
|
||
[s]r=attinas for patches 42870 and 42871 (if it is not too late).
Assignee | ||
Comment 28•23 years ago
|
||
Fix checked in, trunk only.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•23 years ago
|
Summary: Most links on this site crash Mozilla → {ib} Most links on this site crash Mozilla
Comment 29•23 years ago
|
||
should we get this on 0.9.2 branch also?
Assignee | ||
Comment 30•23 years ago
|
||
N.B. that this fix regresses bug 41521; however, since that was a made-up bug (no real-world test case AFAIK), I think the crash fix would be better. I'll re-open for consideration on the branch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 31•23 years ago
|
||
so is this ok on 0.9.4 and the current trunk? if so lets close, since we aren't applying patches to the 092 branch anymore.
Assignee | ||
Updated•23 years ago
|
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 32•23 years ago
|
||
Yes.
Comment 33•15 years ago
|
||
Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/5a6def05ccbc
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•