296450 - Object Dom Node does not work anymore

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050602
Firefox/1.0+ ID:2005060213

1.open testcase (just to keep it simple)
2.open Domi
3.got to IMG
4.notice Node name in rightscreen shows the title 4 times, no style, alt, src

this regressed between the 
20050601 0740pdt build (works)
and
20050601 0940pdt build (borked)

approximate checkinlist for that period (the checkins during building included)
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-06-01+06%3A10%3A00&maxdate=2005-06-01+09%3A40%3A00&cvsroot=%2Fcvsroot

testcase next

Comment 1

[Peter van der Woude [:Peter6]]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

My money is on the nsDOMAttributeMap changes from bug 235512.  On the testcase,
the following:

javascript:alert(document.getElementsByTagName("img")[0].attributes.item(2) ==
                 document.getElementsByTagName("img")[0].attributes.item(1))

alerts true...

Comment 4

[Peter Van der Beken [:peterv]]

Could someone confirm that this is still broken in builds that have the fix for
bug 296490?

Comment 5

[Caleb]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050608
Firefox/1.0+

I still see this...

Comment 6

[Peter Van der Beken [:peterv]]

I can't reproduce this on OS X and bz mentioned he couldn't reproduce on Linux :-/.

Comment 7

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050608
Firefox/1.0+ ID:2005060809

This is still broken on Windoze

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

I had a patch in my tree that appears to fix this, but it also includes some
other stuff. I think the problem is that the hash in nsDOMAttributeMap passes
around nsAttrKey rather then nsAttrKey&. This at least screws up
nsAttrHashKey::KeyToPointer since you're returning a pointer to the local
variable within that function (this generates a warning which is how I found
it). An alternative fix could have just been to make KeyToPointer take a
KeyType&. This works fine here, but nsTHashtable.h seems to recommend against
it.

The patch also contains the fixe for bug 296207 as well as a fix to make
.ownerDocument work in all situations. I can separate out that stuff if needed.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 185736 [details] [diff] [review]
Patch to fix

I'd much rather see this split up, so sr=peterv on these changes (I think I got
them all):

>Index: base/src/nsDOMAttributeMap.cpp
>===================================================================

>@@ -121,13 +139,13 @@ nsDOMAttributeMap::GetAttribute(nsINodeI
> {
>   NS_ASSERTION(aNodeInfo, "GetAttribute() called with aNodeInfo == nsnull!");
>   NS_ASSERTION(aReturn, "GetAttribute() called with aReturn == nsnull");
> 
>   *aReturn = nsnull;
> 
>-  nsAttrHashKey::KeyType attr(aNodeInfo->NamespaceID(), aNodeInfo->NameAtom());
>+  nsAttrKey attr(aNodeInfo->NamespaceID(), aNodeInfo->NameAtom());
> 
>   if (!mAttributeCache.Get(attr, aReturn)) {
>     nsAutoString value;
>     if (aRemove) {
>       // As we are removing the attribute we need to set the current value in
>       // the attribute node.

>Index: base/src/nsDOMAttributeMap.h
>===================================================================

>@@ -76,13 +77,13 @@ public:
> /**
>  * PLDHashEntryHdr implementation for nsAttrKey.
>  */
> class nsAttrHashKey : public PLDHashEntryHdr
> {
> public:
>-  typedef nsAttrKey KeyType;
>+  typedef const nsAttrKey& KeyType;
>   typedef const nsAttrKey* KeyTypePointer;
> 
>   nsAttrHashKey(KeyTypePointer aKey) : mKey(*aKey) {}
>   nsAttrHashKey(const nsAttrHashKey& aCopy) : mKey(aCopy.mKey) {}
>   ~nsAttrHashKey() {}
> 
>@@ -104,13 +105,13 @@ public:
>              (aKey->mNamespaceID << 4) ^
>              NS_PTR_TO_INT32(aKey->mLocalName);
>     }
>   enum { ALLOW_MEMMOVE = PR_TRUE };
> 
> private:
>-  KeyType mKey;
>+  nsAttrKey mKey;
> };
> 
> // Helper class that implements the nsIDOMNamedNodeMap interface.
> class nsDOMAttributeMap : public nsIDOMNamedNodeMap
> {
> public:

Comment 10

[Allan Beaufour]

Comment on attachment 185736 [details] [diff] [review]
Patch to fix

(In reply to comment #8)
> I think the problem is that the hash in nsDOMAttributeMap passes
> around nsAttrKey rather then nsAttrKey&. This at least screws up
> nsAttrHashKey::KeyToPointer since you're returning a pointer to the local
> variable within that function (this generates a warning which is how I found
> it).

I cannot reproduce it either, but I can follow the reasoning all right so r+ on
the same stuff that peterv has sr+'ed.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Separated patch

There were a couple of more blocks, but they're just the same as the parts you
guys reviewed.

Requestiong approval, DOM-Inspector and some aspects of the DOM are broken on
windows without this.

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 185815 [details] [diff] [review]
Separated patch

a=caillon for 1.8b3

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in

Comment 14

[Ria Klaassen (not reading all bugmail)]

Every time when I open the DOM inspector I get two messages in the JS console:

No chrome package registered for chrome://communicator/content/utilityOverlay.xul .
No chrome package registered for chrome://communicator/content/tasksOverlay.xul .

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Seems unrelated to this bug