[webkit-changes] cvs commit: WebCore/khtml/ecma kjs_dom.cpp

Adele adele at opensource.apple.com
Tue Jun 7 15:12:58 PDT 2005


adele       05/06/07 15:12:57

  Modified:    .        ChangeLog
               khtml/ecma kjs_dom.cpp
  Log:
          Change by Darin, reviewed by Maciej and me.
  
          - fixed <rdar://problem/4139800> REGRESSION: Safari crashes at bebe.com
  
          There were cases where the DOMNode mark function would end up not even
          marking the node that it was called on. The old code tried to skip any
          subtrees that were already marked, but that code was wrong.
  
          * khtml/ecma/kjs_dom.cpp: (KJS::DOMNode::mark): Changed three things:
            1) Instead of a boolean, keep a set of trees that we are marking, that
               prevents problems if we mark things that have references between
               separate trees of DOM nodes.
            2) Don't do the "outermost node with a wrapper" check, just start marking
               at the root of the entire tree, because there's no way to have an
               unreachable node pointing to a reachable node.
            3) Handle the unusual case where the document's wrapper dictionary is
               gone by marking just this node explicitly.
          This passes the layout tests, still fixes the Dashboard memory leak from
          bug 4125222, and makes bebe.com work again.
  
  Revision  Changes    Path
  1.4248    +22 -0     WebCore/ChangeLog
  
  Index: ChangeLog
  ===================================================================
  RCS file: /cvs/root/WebCore/ChangeLog,v
  retrieving revision 1.4247
  retrieving revision 1.4248
  diff -u -r1.4247 -r1.4248
  --- ChangeLog	7 Jun 2005 21:48:03 -0000	1.4247
  +++ ChangeLog	7 Jun 2005 22:12:54 -0000	1.4248
  @@ -1,3 +1,25 @@
  +2005-06-07  Adele Peterson  <adele at apple.com>
  +
  +        Change by Darin, reviewed by Maciej and me.
  +
  +        - fixed <rdar://problem/4139800> REGRESSION: Safari crashes at bebe.com
  +
  +        There were cases where the DOMNode mark function would end up not even
  +        marking the node that it was called on. The old code tried to skip any
  +        subtrees that were already marked, but that code was wrong.
  +
  +        * khtml/ecma/kjs_dom.cpp: (KJS::DOMNode::mark): Changed three things:
  +          1) Instead of a boolean, keep a set of trees that we are marking, that
  +             prevents problems if we mark things that have references between
  +             separate trees of DOM nodes.
  +          2) Don't do the "outermost node with a wrapper" check, just start marking
  +             at the root of the entire tree, because there's no way to have an
  +             unreachable node pointing to a reachable node.
  +          3) Handle the unusual case where the document's wrapper dictionary is
  +             gone by marking just this node explicitly.
  +        This passes the layout tests, still fixes the Dashboard memory leak from
  +        bug 4125222, and makes bebe.com work again.
  +
   2005-06-07  Darin Adler  <darin at apple.com>
   
           Change by Toby Peterson <toby at opendarwin.org>.
  
  
  
  1.74      +38 -18    WebCore/khtml/ecma/kjs_dom.cpp
  
  Index: kjs_dom.cpp
  ===================================================================
  RCS file: /cvs/root/WebCore/khtml/ecma/kjs_dom.cpp,v
  retrieving revision 1.73
  retrieving revision 1.74
  diff -u -r1.73 -r1.74
  --- kjs_dom.cpp	31 May 2005 01:20:46 -0000	1.73
  +++ kjs_dom.cpp	7 Jun 2005 22:12:57 -0000	1.74
  @@ -140,36 +140,56 @@
   
   void DOMNode::mark()
   {
  -  static bool markingTree = false;
  +  assert(!marked());
   
  -  if (m_impl->inDocument() || markingTree) {
  +  NodeImpl *node = m_impl.get();
  +
  +  // Nodes in the document are kept alive by ScriptInterpreter::mark,
  +  // so we have no special responsibilities and can just call the base class here.
  +  if (node->inDocument()) {
       DOMObject::mark();
       return;
     }
   
  -  DocumentImpl *document = m_impl->getDocument();
  -  NodeImpl *outermostWrappedNode = m_impl.get();
  -  for (NodeImpl *current = m_impl->parentNode(); current; current = current->parentNode()) {
  -    if (ScriptInterpreter::getDOMNodeForDocument(document, current))
  -      outermostWrappedNode = current;
  +  // This is a node outside the document, so find the root of the tree it is in,
  +  // and start marking from there.
  +  NodeImpl *root = node;
  +  for (NodeImpl *current = m_impl.get(); current; current = current->parentNode()) {
  +    root = current;
     }
   
  -  markingTree = true;
  +  static QPtrDict<NodeImpl> markingRoots;
  +
  +  // If we're already marking this tree, then we can simply mark this wrapper
  +  // by calling the base class; our caller is iterating the tree.
  +  if (markingRoots.find(root)) {
  +    DOMObject::mark();
  +    return;
  +  }
  +
  +  DocumentImpl *document = m_impl->getDocument();
   
  -  NodeImpl *nodeToMark = outermostWrappedNode;
  -  while (nodeToMark) {
  +  // Mark the whole tree; use the global set of roots to avoid reentering.
  +  markingRoots.insert(root, root);
  +  for (NodeImpl *nodeToMark = root; nodeToMark; nodeToMark = nodeToMark->traverseNextNode()) {
       DOMNode *wrapper = ScriptInterpreter::getDOMNodeForDocument(document, nodeToMark);
  -    if (!wrapper)
  -      nodeToMark = nodeToMark->traverseNextNode();
  -    else if (wrapper->marked())
  -      nodeToMark = nodeToMark->traverseNextSibling();
  -    else {
  -      wrapper->mark();
  -      nodeToMark = nodeToMark->traverseNextNode();
  +    if (wrapper) {
  +      if (!wrapper->marked())
  +        wrapper->mark();
  +    } else if (nodeToMark == node) {
  +      // This is the case where the map from the document to wrappers has
  +      // been cleared out, but a wrapper is being marked. For now, we'll
  +      // let the rest of the tree of wrappers get collected, because we have
  +      // no good way of finding them. Later we should test behavior of other
  +      // browsers and see if we need to preserve other wrappers in this case.
  +      if (!marked())
  +        mark();
       }
     }
  +  markingRoots.remove(root);
   
  -  markingTree = false;
  +  // Double check that we actually ended up marked. This assert caught problems in the past.
  +  assert(marked());
   }
   
   bool DOMNode::toBoolean(ExecState *) const
  
  
  



More information about the webkit-changes mailing list