55095 - Stack overflow causes crash

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Mike Pinkerton (not reading bugmail)]

- go to www.hpefcu.com/hpmain.html

crash in PurgeSpace() called from nsDrawingSurfaceMac::Init(), heap is trashed. 
Happens 100% of the time on this page, looks like it's trying to layout the 
rightmost frame when it dies.

Comment 1

[Mike Pinkerton (not reading bugmail)]

note, works fine on win32. this is the 9/29/00 branch fc build.

this is dogfood for me, as it's my banking page. having this crash will make me 
use 4.x.

Comment 2

[Simon Fraser [no longer active]]

This looks like some bad code in the disk cache, calling FspGetFullPath, and 
treating the handle badly.

Comment 3

[rubydoo123]

nomination accepted

Comment 4

[Simon Fraser [no longer active]]

Well, that disk cache suggestion seems to be wrong; I can't reliably debug the 
point where the corruption occurs, despite trying the NewPtr allocators.\

PDT: this is a serious bug and needs to be fixed. It just needs more engineer 
time to debug.

Comment 5

[Simon Fraser [no longer active]]

This is a stack overalow problem; that left frame has 160-odd bogus tags in the <
head> that seem to cause bad things to happen.

Layout really needs to do a better job of avoiding stack overflow. Over to them.

Comment 6

[Simon Fraser [no longer active]]

Attachment: Minimal test case derived from the original page. May need to resize browser window to see crash.

Comment 7

[Simon Fraser [no longer active]]

-> layou.t Minimal test case attached.

Comment 8

[Simon Fraser [no longer active]]

cc self.

Comment 9

[buster]

assigning to rickg.  suggest we try to fix for rtm.  Rick, it seems your parser
"fix" for the unclosed inline tag problem either doesn't work with tags in the
head, or doesn't work with unknown tag types which default to display: inline.

Note that this problem occurs because the site is using an illegal tag that has
neither end tags nor a style sheet to set the unknown tag to display: none.  We
can't just evangelize away the problem, however, since we need to generally
protect ourselves from crashing in this sort of senario.

Rick, if you don't think the parser should handle this situation like the other
stack overflow problems, send it back to me and I can add stack depth protection
into nsCSSFrameConstructor directly.  That's a slightly higher risk solution,
though.

Comment 10

[Doron Rosenberg (IBM)]

setting default qa

Comment 11

[rickg]

I'm looking now, but recall that I've been saying that the depth I allow was 
chosen for Windows, and that we needed input from a mac guy to help set the 
right level on that platform. That *may* be all that is required -- but I'll do 
some testing and report back.

Comment 12

[rickg]

The stack depth gate was only designed to handle inlines, thus user_defined tags 
were not considered. So now I'm wondering whether I should use this gate for all 
types (block, inline, etc).

Comment 13

[rickg]

I've decided to do this for all containers -- and gate the max depth at 75. I 
picked 75 after ChrisP and I tested the mac and saw crashes in the low 80's.
R,SR=buster.

Comment 14

[Simon Fraser [no longer active]]

Can we see a diff here in the bug please, before we go ahead with r= and sr=

Comment 15

[rickg]

The original code in parser was:

  if(MAX_REFLOW_DEPTH<mBodyContext->GetCount()) {
    if(gHTMLElements[aTag].IsMemberOf(kInlineEntity)) {
      if(!gHTMLElements[aTag].IsMemberOf(kFormControl)) {
        return kHierarchyTooDeep;
      }
    }
  }

The new version is:

  if(MAX_REFLOW_DEPTH<mBodyContext->GetCount()) {
    return kHierarchyTooDeep;
  }

We'll also set the MAX_REFLOW_DEPTH to 75.

Comment 16

[rickg]

The original code in CNavDTD.cpp (line 1195) was:

  if(MAX_REFLOW_DEPTH<mBodyContext->GetCount()) {
    if(gHTMLElements[aTag].IsMemberOf(kInlineEntity)) {
      if(!gHTMLElements[aTag].IsMemberOf(kFormControl)) {
        return kHierarchyTooDeep;
      }
    }
  }

The new version is:

  if(MAX_REFLOW_DEPTH<mBodyContext->GetCount()) {
    return kHierarchyTooDeep;
  }

We'll also set the MAX_REFLOW_DEPTH to 75.

Comment 17

[rickg]

I've attached the patch.

Comment 18

[Jim Roskind]

Simon asked for to see the patch attached... and rick said he attached it... but 
I don't see it.
Rick: Could you supply the patch as an attachment?

Comment 19

[rickg]

JAR: see my last two comments. They are the before and after which fix the 
problem. If you really need a diff -- I'll append that, but what I've provided 
should be sufficient. 

Comment 20

[rickg]

The code has been reviewed and superreviewed by buster.

Comment 21

[Phil Peterson]

PDT marking [rtm++]. RickG is one step ahead of the PDT, as usual!

Comment 22

[Simon Fraser [no longer active]]

That fixes the crash in the URL testcase here, but it also causes the left-hand 
pane to not show up at all (with MAX_REFLOW_DEPTH == 75).

Also, the #define MAX_REFLOW_DEPTH  200 is in nsIHTMLContentSink.h, but only used 
in CNavDTD.cpp. There is another #define of the same name in frames:
http://lxr.mozilla.org/seamonkey/search?string=MAX_REFLOW_DEPTH
Do we need to fix that too? What about COtherDTD.cpp?

I think the proposed patch is suboptimal, as it stands.

Comment 23

[Jim Roskind]

Alas, this isn't Netscape's Credit Union... so I'm marking dogfood-minus (sorry
pink).

Rick: Please be sure to get closure on the review with Simon.  Note that
super-reviewer and reviewer are supposed to be distinct.  Adding the patch would
clarify Simon's question of exactly what is getting changed.

Comment 24

[rickg]

Simon -- take your pick: either live with the crash, or with the possibility 
that we'll stop building structure when we reach a max depth. 

As for the 2nd MAX_REFLOW_DEPTH in frame, buster was going to remove it. I'll go 
talk with him.

Comment 25

[rickg]

Here's the scoop:

We've known all along that putting this gating function in the parser is a short 
term solution. Given the date, it's all buster and I we're willing to risk. It 
does catch the problem of excessively nested structures on the web today. It 
*WONT* catch excessively nested structures provided via the DOM. 

Our choices are: 1) do nothing; 2) do this simple patch; 3) try to improve the 
frame construction code to avoid stack overflow. I'm still in favor of doing 
this now, and then working on #3 for a better solution post RTM.

BTW: Buster forgot to check in the use of MAX_REFLOW_DEPTH in nsFrame, and has 
adjusted his build accordingly. After some testing, I'll add that change to this 
patch as well.

Comment 26

[buster]

what rick said.
We in no way intend for this to be the long-term solution. Frame construction
and reflow both need to be examined to reduce stack usage, with frame
construction being the bigger offender these days. The basic frame construction
algorithm is recursive. Perhaps we could look into making it iterative, or heap
allocating some data, or whatever we need to do. Reflow already does the heap
allocation trick: for it's major data structure, it stack allocates the first n
(I think 30), then heap allocates all subsequent calls. We could do some dynamic
stack-measurement tricks to both optimize this and to protect against overruns.
But none of this will happen for rtm.

Comment 27

[Mike Pinkerton (not reading bugmail)]

even though i hate the idea of content not being rendered, i hate the idea of
having to quit 6.0 and use a different browser to check my savings account.

i'd rather have no content than a crash (for the short term). is there a bug on
doing this the right way for the long term?

Comment 28

[Simon Fraser [no longer active]]

OK, with those caveats, r=sfraser.

Comment 29

[rickg]

Pink: no, there's not, so I'll open one on myself. And thanks for the input, 
all.

Comment 30

[rickg]

Added bug 56182 to correct the *real* problem this hack is dancing around.

Comment 31

[rickg]

Semi-hack fix involving a gating function in the parser which prevents 
excessively nested structures from being sent to content sink. This is an RTM 
fix only (and mostly affects mac). The right way to handle this is discussed in 
bug 56182.

Comment 32

[Chris Petersen]

Waiting for mac branch build (Sept 12th) to verify.

Comment 33

[rickg]

*** Bug 49678 has been marked as a duplicate of this bug. ***

Comment 34

[Chris Petersen]

Fixed in the Mac branch build (2000101214).

Comment 35

[olga charapaev]

I'm trying to verify  this bug in vtrunk using minimized test case (build id 
2000102408). 
Crash has been eliminated. 
But I don't see the word "Hello" which suppose to display in the window. 
Branch build Gecko/20001024 Netscape6 does not display it either.
Is this a side effect "the left-hand pane  not show up at all..." you mentioned 
in this bug report? 

Comment 36

[Asa Dotzler [:asa]]

rickg, you said this was an RTM only fix.  Did it land in the trunk?  Is not
crashing on testcase sufficient to Verify this on teh trunk?

Comment 37

[rickg]

It landed on both; the text should'nt be missing, but that's a different bug 
that won't land until 6.01. 

Comment 38

[olga charapaev]

OK then, marking the bug as VERIFIED,
removing vtrunk keyword.