90205 - {ib} Most links on this site crash Mozilla

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Sam Rowe]

Build ID: 2001071008

Comment 1

[Asa Dotzler [:asa]]

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

[Asa Dotzler [:asa]]

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. 

Comment 3

[Sam Rowe]

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.

Comment 4

[Sam Rowe]

http://store.yahoo.com/sohowaredirect/ncp600.html

is a crasher...

Comment 5

[Asa Dotzler [:asa]]

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

[Asa Dotzler [:asa]]

over to stylesystem. 

Comment 7

[Pierre Saslawsky]

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.

Comment 8

[Chris Waterson]

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?

Comment 9

[Chris Waterson]

...yup. The above link crashes me on both Mac and Linux, but not Win32.

Comment 10

[Chris Waterson]

Attachment: minimized test case

Comment 11

[Chris Waterson]

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.

Comment 12

[Chris Waterson]

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.

Comment 13

[Chris Waterson]

...or, maybe we should just not reconstruct the frame tree if all that's being
requested here is a reflow.

Comment 14

[Chris Waterson]

Attachment: Remove call to ReframeContainingBlock() from nsCSSFrameConstructor::StyleChangeReflow()

Comment 15

[Chris Waterson]

``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.

Comment 16

[Chris Waterson]

Attachment: better patch; if IsFrameSpecial(), target style change reflow at containing block

Comment 17

[Chris Waterson]

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

[Marc Attinasi]

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

[Blake Ross]

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) ;)

Comment 20

[Chris Waterson]

Attachment: one more try, but don't go out to floater containing block

Comment 21

[Chris Waterson]

Attachment: more modest patch for consideration on mozilla-0.9.2 branch

Comment 22

[Chris Waterson]

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].

Comment 23

[Chris Waterson]

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

[karnaze (gone)]

r=karnaze

Comment 25

[rbs]

>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).

Comment 26

[Chris Waterson]

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

[Marc Attinasi]

[s]r=attinas for patches 42870 and 42871 (if it is not too late).

Comment 28

[Chris Waterson]

Fix checked in, trunk only.

Comment 29

[Marek Z. Jeziorek]

should we get this on 0.9.2 branch also?

Comment 30

[Chris Waterson]

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.

Comment 31

[chris hofmann]

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.

Comment 32

[Chris Waterson]

Yes.

Comment 33

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/5a6def05ccbc