[Webkit-unassigned] [Bug 18282] WebKit crashes with deeply nested divs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=18282


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52852|review?                     |review-
               Flag|                            |




--- Comment #47 from Darin Adler <darin at apple.com>  2010-04-09 09:14:19 PST ---
(From update of attachment 52852)
> +#ifndef config_h
> +#define config_h

The config.h file is not a normal header file, and should not have a header
guard in it. We can discuss this further elsewhere, but please don't add this
in this patch without further discussion.

> +static const unsigned maxDOMTreeDepth = MAX_DOM_TREE_DEPTH;

It's not appropriate to have a "static const" in the config.h file.

I didn't review the rest of the patch, but these are both no-nos.

I don't think this value should go in config.h. It should go in a header file
included only by XMLTokenizer.cpp and HTMLParser.cpp. We don't want this to
have any effect on the rest of the source code. We could add a new source file
called TreeDepthLimit.h in the dom directory.

I think that m_nodeDepth is a strange name; is it really the depth of a node?

The patch is risky. If we find that in some cases the node depth gets off, then
we could either underflow to 0 or slowly creep up to the maximum depth and
break parsing. So we need to make sure we are testing all the parsing code
paths. It would help to have an assertion that the depth is back to zero once
parsing is done. Running the regression tests with such an assert in place
could help us catch any mismatches that otherwise would be silent.

Besides that, I think we need to do some performance testing, but otherwise,
this patch looks good to me.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.