Bug 481440. Make our id table always live. r=jst, sr=roc

2009-03-23 10:04:40 -04:00 · 2009-03-23 10:04:40 -04:00 · a136e8c6f9
--- a/content/base/src/nsDocument.cpp
+++ b/content/base/src/nsDocument.cpp
@ -304,7 +304,6 @@ nsUint32ToContentHashEntry::VisitContent(Visitor* aVisitor)
  }
 }

-#define ID_NOT_IN_DOCUMENT ((nsIContent *)2)
 #define NAME_NOT_VALID ((nsBaseContentList*)1)

 nsIdentifierMapEntry::~nsIdentifierMapEntry()
@ -349,13 +348,9 @@ nsIdentifierMapEntry::CreateNameContentList()
 }

 nsIContent*
-nsIdentifierMapEntry::GetIdContent(PRBool* aNotInDocument)
+nsIdentifierMapEntry::GetIdContent()
 {
-  nsIContent* c = static_cast<nsIContent*>(mIdContentList.SafeElementAt(0));
-  if (aNotInDocument) {
-    *aNotInDocument = c == ID_NOT_IN_DOCUMENT;
-  }
-  return c != ID_NOT_IN_DOCUMENT ? c : nsnull;
+  return static_cast<nsIContent*>(mIdContentList.SafeElementAt(0));
 }

 void
@ -424,28 +419,23 @@ nsIdentifierMapEntry::AddIdContent(nsIContent* aContent)
  NS_PRECONDITION(aContent, "Must have content");
  NS_PRECONDITION(mIdContentList.IndexOf(nsnull) < 0,
                  "Why is null in our list?");
-  NS_PRECONDITION(aContent != ID_NOT_IN_DOCUMENT,
-                  "Bogus content pointer");

-  nsIContent* currentContent = static_cast<nsIContent*>(mIdContentList.SafeElementAt(0));
-  if (currentContent == ID_NOT_IN_DOCUMENT) {
-    NS_ASSERTION(mIdContentList.Count() == 1, "Bogus count");
-    mIdContentList.ReplaceElementAt(aContent, 0);
-    FireChangeCallbacks(nsnull, aContent);
-    return PR_TRUE;
-  }
+#ifdef DEBUG
+  nsIContent* currentContent =
+    static_cast<nsIContent*>(mIdContentList.SafeElementAt(0));
+#endif

  // Common case
  if (mIdContentList.Count() == 0) {
    if (!mIdContentList.AppendElement(aContent))
      return PR_FALSE;
+    NS_ASSERTION(currentContent == nsnull, "How did that happen?");
    FireChangeCallbacks(nsnull, aContent);
    return PR_TRUE;
  }

-  // We seem to have multiple content nodes for the same id, or we're doing our
-  // top-down registration when the id table is going live.  Search for the
-  // right place to insert the content.
+  // We seem to have multiple content nodes for the same id, or XUL is messing
+  // with us.  Search for the right place to insert the content.
  PRInt32 start = 0;
  PRInt32 end = mIdContentList.Count();
  do {
@ -457,6 +447,8 @@ nsIdentifierMapEntry::AddIdContent(nsIContent* aContent)
    nsIContent* curContent = static_cast<nsIContent*>(mIdContentList[cur]);
    if (curContent == aContent) {
      // Already in the list, so already in the right spot.  Get out of here.
+      // XXXbz this only happens because XUL does all sorts of random
+      // UpdateIdTableEntry calls.  Hate, hate, hate!
      return PR_TRUE;
    }

@ -470,7 +462,10 @@ nsIdentifierMapEntry::AddIdContent(nsIContent* aContent)
  if (!mIdContentList.InsertElementAt(aContent, start))
    return PR_FALSE;
  if (start == 0) {
-    FireChangeCallbacks(currentContent, aContent);
+    nsIContent* oldContent =
+      static_cast<nsIContent*>(mIdContentList.SafeElementAt(1));
+    NS_ASSERTION(currentContent == oldContent, "How did that happen?");
+    FireChangeCallbacks(oldContent, aContent);
  }
  return PR_TRUE;
 }
@ -493,15 +488,6 @@ nsIdentifierMapEntry::RemoveIdContent(nsIContent* aContent)
  return mIdContentList.Count() == 0 && !mNameContentList && !mChangeCallbacks;
 }

-void
-nsIdentifierMapEntry::FlagIDNotInDocument()
-{
-  NS_ASSERTION(mIdContentList.Count() == 0,
-               "Flagging ID not in document when we have content?");
-  // Note that if this fails that's OK; this is just an optimization
-  mIdContentList.AppendElement(ID_NOT_IN_DOCUMENT);
-}
-
 void
 nsIdentifierMapEntry::AddNameContent(nsIContent* aContent)
 {
@ -2307,11 +2293,9 @@ nsDocument::UpdateIdTableEntry(nsIContent *aContent)
  if (!id)
    return;

-  PRBool liveTable = IdTableIsLive();
-  nsIdentifierMapEntry *entry =
-    liveTable ? mIdentifierMap.PutEntry(id) : mIdentifierMap.GetEntry(id);
+  nsIdentifierMapEntry *entry = mIdentifierMap.PutEntry(id);

-  if (entry) {
+  if (entry) { /* True except on OOM */
    entry->AddIdContent(aContent);
  }
 }
@ -2324,7 +2308,7 @@ nsDocument::RemoveFromIdTable(nsIContent *aContent)
    return;

  nsIdentifierMapEntry *entry = mIdentifierMap.GetEntry(id);
-  if (!entry)
+  if (!entry) /* Should be false unless we had OOM when adding the entry */
    return;

  if (entry->RemoveIdContent(aContent)) {
@ -2335,35 +2319,51 @@ nsDocument::RemoveFromIdTable(nsIContent *aContent)
 void
 nsDocument::UnregisterNamedItems(nsIContent *aContent)
 {
-  if (aContent->IsNodeOfType(nsINode::eTEXT)) {
-    // Text nodes are not named items nor can they have children.
+  if (!aContent->IsNodeOfType(nsINode::eELEMENT)) {
+    // non-element nodes are not named items nor can they have children.
    return;
  }

  RemoveFromNameTable(aContent);
  RemoveFromIdTable(aContent);

-  PRUint32 i, count = aContent->GetChildCount();
-  for (i = 0; i < count; ++i) {
-    UnregisterNamedItems(aContent->GetChildAt(i));
+#ifdef DEBUG
+  nsMutationGuard debugMutationGuard;
+#endif
+
+  PRUint32 count;
+  nsIContent * const * kidSlot = aContent->GetChildArray(&count);
+  nsIContent * const * end = kidSlot + count;
+  for (; kidSlot != end; ++kidSlot) {
+    UnregisterNamedItems(*kidSlot);
  }
+
+  NS_ASSERTION(!debugMutationGuard.Mutated(0), "Unexpected mutations happened");
 }

 void
 nsDocument::RegisterNamedItems(nsIContent *aContent)
 {
-  if (aContent->IsNodeOfType(nsINode::eTEXT)) {
-    // Text nodes are not named items nor can they have children.
+  if (!aContent->IsNodeOfType(nsINode::eELEMENT)) {
+    // non-element nodes are not named items nor can they have children.
    return;
  }

  UpdateNameTableEntry(aContent);
  UpdateIdTableEntry(aContent);

-  PRUint32 i, count = aContent->GetChildCount();
-  for (i = 0; i < count; ++i) {
-    RegisterNamedItems(aContent->GetChildAt(i));
+#ifdef DEBUG
+  nsMutationGuard debugMutationGuard;
+#endif
+
+  PRUint32 count;
+  nsIContent * const * kidSlot = aContent->GetChildArray(&count);
+  nsIContent * const * end = kidSlot + count;
+  for (; kidSlot != end; ++kidSlot) {
+    RegisterNamedItems(*kidSlot);
  }
+
+  NS_ASSERTION(!debugMutationGuard.Mutated(0), "Unexpected mutations happened");
 }

 void
@ -2373,10 +2373,18 @@ nsDocument::ContentAppended(nsIDocument* aDocument,
 {
  NS_ASSERTION(aDocument == this, "unexpected doc");

-  PRUint32 count = aContainer->GetChildCount();
-  for (PRUint32 i = aNewIndexInContainer; i < count; ++i) {
-    RegisterNamedItems(aContainer->GetChildAt(i));
+#ifdef DEBUG
+  nsMutationGuard debugMutationGuard;
+#endif
+
+  PRUint32 count;
+  nsIContent * const * kidSlot = aContainer->GetChildArray(&count);
+  nsIContent * const * end = kidSlot + count;
+  for (kidSlot += aNewIndexInContainer; kidSlot != end; ++kidSlot) {
+    RegisterNamedItems(*kidSlot);
  }
+
+  NS_ASSERTION(!debugMutationGuard.Mutated(0), "Unexpected mutations happened");
 }

 void
@ -3754,19 +3762,6 @@ nsDocument::CheckGetElementByIdArg(const nsIAtom* aId)
  return PR_TRUE;
 }

-static void
-MatchAllElementsId(nsIContent* aContent, nsIAtom* aId, nsIdentifierMapEntry* aEntry)
-{
-  if (aId == aContent->GetID()) {
-    aEntry->AddIdContent(aContent);
-  }
-
-  PRUint32 i, count = aContent->GetChildCount();
-  for (i = 0; i < count; i++) {
-    MatchAllElementsId(aContent->GetChildAt(i), aId, aEntry);
-  }
-}
-
 nsIdentifierMapEntry*
 nsDocument::GetElementByIdInternal(nsIAtom* aID)
 {
@ -3780,10 +3775,9 @@ nsDocument::GetElementByIdInternal(nsIAtom* aID)
  if (entry->GetIdContent())
    return entry;

-  // Now we have to flush.  It could be that we have a cached "not in
-  // document" or know nothing about this ID yet but more content has been
-  // added to the document since.  Note that we have to flush notifications,
-  // so that the entry will get updated properly.
+  // Now we have to flush.  It could be that we know nothing about this ID yet
+  // but more content has been added to the document since.  Note that we have
+  // to flush notifications, so that the entry will get updated properly.

  // Make sure to stash away the current generation so we can check whether
  // the table changes when we flush.
@ -3798,53 +3792,6 @@ nsDocument::GetElementByIdInternal(nsIAtom* aID)
    entry = mIdentifierMap.PutEntry(aID);
  }
  
-  PRBool isNotInDocument;
-  nsIContent *e = entry->GetIdContent(&isNotInDocument);
-  if (e || isNotInDocument)
-    return entry;
-
-  // Status of this id is unknown, search document
-  nsIContent* root = GetRootContent();
-  if (!IdTableIsLive()) {
-    if (IdTableShouldBecomeLive()) {
-      // Just make sure our table is up to date and call this method again
-      // to look up in the hashtable.
-      if (root) {
-        RegisterNamedItems(root);
-      }
-      return GetElementByIdInternal(aID);
-    }
-
-    if (root) {
-      // No-one should have registered an ID change callback yet. We don't
-      // want to fire one as a side-effect of getElementById! This shouldn't
-      // happen, since if someone called AddIDTargetObserver already for
-      // this ID, we should have filled in this entry with content or
-      // not-in-document.
-      NS_ASSERTION(!entry->HasContentChangeCallback(),
-                   "No callbacks should be registered while we set up this entry");
-      MatchAllElementsId(root, aID, entry);
-      e = entry->GetIdContent();
-    }
-  }
-
-  if (!e) {
-#ifdef DEBUG
-    // No reason to call MatchElementId if !IdTableIsLive, since
-    // we'd have done just that already
-    if (IdTableIsLive() && root && aID != nsGkAtoms::_empty) {
-      nsIContent* eDebug =
-        nsContentUtils::MatchElementId(root, aID);
-      NS_ASSERTION(!eDebug,
-                   "We got null for |e| but MatchElementId found something?");
-    }
-#endif
-    // There is no element with the given id in the document, cache
-    // the fact that it's not in the document
-    entry->FlagIDNotInDocument();
-    return entry;
-  }
-
  return entry;
 }

@ -3863,10 +3810,8 @@ nsDocument::GetElementById(const nsAString& aElementId,
  nsIdentifierMapEntry *entry = GetElementByIdInternal(idAtom);
  NS_ENSURE_TRUE(entry, NS_ERROR_OUT_OF_MEMORY);

-  PRBool isNotInDocument;
-  nsIContent *e = entry->GetIdContent(&isNotInDocument);
-  NS_ASSERTION(e || isNotInDocument, "Incomplete map entry!");
-  if (isNotInDocument)
+  nsIContent *e = entry->GetIdContent();
+  if (!e)
    return NS_OK;

  return CallQueryInterface(e, aReturn);
--- a/content/base/src/nsDocument.h
+++ b/content/base/src/nsDocument.h
@ -257,10 +257,11 @@ public:
  /**
   * Returns the element if we know the element associated with this
   * id. Otherwise returns null.
-   * @param aIsNotInDocument if non-null, we set the output to true
-   * if we know for sure the element is not in the document.
   */
-  nsIContent* GetIdContent(PRBool* aIsNotInDocument = nsnull);
+  nsIContent* GetIdContent();
+  /**
+   * Append all the elements with this id to aElements
+   */
  void AppendAllIdContent(nsCOMArray<nsIContent>* aElements);
  /**
   * This can fire ID change callbacks.
@ -273,7 +274,6 @@ public:
   * @return true if this map entry should be removed
   */
  PRBool RemoveIdContent(nsIContent* aContent);
-  void FlagIDNotInDocument();

  PRBool HasContentChangeCallback() { return mChangeCallbacks != nsnull; }
  void AddContentChangeCallback(nsIDocument::IDTargetObserver aCallback, void* aData);
@ -318,8 +318,7 @@ public:
 private:
  void FireChangeCallbacks(nsIContent* aOldContent, nsIContent* aNewContent);

-  // The single element ID_NOT_IN_DOCUMENT, or empty to indicate we
-  // don't know what element(s) have this key as an ID
+  // empty if there are no nodes with this ID
  nsSmallVoidArray mIdContentList;
  // NAME_NOT_VALID if this id cannot be used as a 'name'
  nsBaseContentList *mNameContentList;
@ -1149,8 +1148,8 @@ protected:
   * 1) Attribute changes affect the table immediately (removing and adding
   *    entries as needed).
   * 2) Removals from the DOM affect the table immediately
-   * 3) Additions to the DOM always update existing entries, but only add new
-   *    ones if IdTableIsLive() is true.
+   * 3) Additions to the DOM always update existing entries for names, and add
+   *    new ones for IDs.
   */
  nsTHashtable<nsIdentifierMapEntry> mIdentifierMap;

@ -1192,22 +1191,6 @@ protected:

  PRUint8 mDefaultElementType;

-  PRBool IdTableIsLive() const {
-    // live if we've had over 63 misses
-    return (mIdMissCount & 0x40) != 0;
-  }
-  void SetIdTableLive() {
-    mIdMissCount = 0x40;
-  }
-  PRBool IdTableShouldBecomeLive() {
-    NS_ASSERTION(!IdTableIsLive(),
-                 "Shouldn't be called if table is already live!");
-    ++mIdMissCount;
-    return IdTableIsLive();
-  }
-
-  PRUint8 mIdMissCount;
-
  nsInterfaceHashtable<nsVoidPtrHashKey, nsPIBoxObject> *mBoxObjectTable;

  // The channel that got passed to StartDocumentLoad(), if any
--- a/content/html/document/src/nsHTMLContentSink.cpp
+++ b/content/html/document/src/nsHTMLContentSink.cpp
@ -2328,7 +2328,7 @@ HTMLContentSink::OpenContainer(const nsIParserNode& aNode)
    case eHTMLTag_head:
      rv = OpenHeadContext();
      if (NS_SUCCEEDED(rv)) {
-        rv = AddAttributes(aNode, mHead, PR_FALSE, mHaveSeenHead);
+        rv = AddAttributes(aNode, mHead, PR_TRUE, mHaveSeenHead);
        mHaveSeenHead = PR_TRUE;
      }
      break;
--- a/content/html/document/src/nsHTMLDocument.cpp
+++ b/content/html/document/src/nsHTMLDocument.cpp
@ -3203,16 +3203,8 @@ nsHTMLDocument::GetDocumentAllResult(const nsAString& aID, nsISupports** aResult
  *aResult = nsnull;

  nsCOMPtr<nsIAtom> id = do_GetAtom(aID);
-  nsIdentifierMapEntry *entry;
-  if (IdTableIsLive()) {
-    entry = mIdentifierMap.GetEntry(id);
-    // If we did a lookup and it failed, there are no items with this id
-    if (!entry)
-      return NS_OK;
-  } else {
-    entry = mIdentifierMap.PutEntry(id);
-    NS_ENSURE_TRUE(entry, NS_ERROR_OUT_OF_MEMORY);
-  }
+  nsIdentifierMapEntry *entry = mIdentifierMap.PutEntry(id);
+  NS_ENSURE_TRUE(entry, NS_ERROR_OUT_OF_MEMORY);

  nsIContent* root = GetRootContent();
  if (!root) {
--- a/content/html/document/test/Makefile.in
+++ b/content/html/document/test/Makefile.in
@ -89,6 +89,7 @@ _TEST_FILES = 	test_bug1682.html \
 		bug448564-iframe-3.html \
 		bug448564-echo.sjs \
 		bug448564-submit.js \
+		test_bug481440.html \
 		test_bug482659.html \
 		$(NULL)

--- a/content/html/document/test/test_bug481440.html
+++ b/content/html/document/test/test_bug481440.html
@ -0,0 +1,31 @@
+<!--Test must be in quirks mode for document.all to work-->
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=481440
+-->
+<head>
+  <title>Test for Bug 481440</title>
+  <script type="application/javascript" src="/MochiKit/MochiKit.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=481440">Mozilla Bug 481440</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+  <input name="x" id="y">
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 481440 **/
+// Do a bunch of getElementById calls to catch hashtables auto-going live
+for (var i = 0; i < 500; ++i) {
+  document.getElementById(i);
+}
+is(document.all["x"], document.getElementById("y"),
+   "Unexpected node");
+</script>
+</pre>
+</body>
+</html>
--- a/content/xul/document/src/nsXULDocument.cpp
+++ b/content/xul/document/src/nsXULDocument.cpp
@ -1948,7 +1948,6 @@ nsXULDocument::CloneNode(PRBool aDeep, nsIDOMNode** aReturn)
 nsresult
 nsXULDocument::Init()
 {
-    SetIdTableLive();
    mRefMap.Init();

    nsresult rv = nsXMLDocument::Init();