231104 - Checkin for Bug 195350 causes AIX build to segfault on startup

Status: RESOLVED FIXED

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

Description

[Philip K. Warren]

After the checkin for Bug 195350, the AIX build segfaults on startup. I have a
detailed stacktrace for the crash which I will post to this bug.

Comment 1

[Philip K. Warren]

Attachment: Stack trace

Here is the stack trace. It appears that the following call in
nsXBLContentSink::ConstructBinding is always returning an empty string, so
mBinding is never being initialized:

binding->GetAttr(kNameSpaceID_None, nsHTMLAtoms::id, id);
-> id is empty after this call

nsXBLContentSink::ConstructBinding(this=0x2ff1f0a8)
GetAttr in nsXBLContentSink::ConstructBinding returning cid=
nsXBLContentSink::ConstructBinding(this=0x2ff1f0a8)
GetAttr in nsXBLContentSink::ConstructBinding returning cid=
nsXBLContentSink::ConstructBinding(this=0x2ff1f0a8)
GetAttr in nsXBLContentSink::ConstructBinding returning cid=
nsXBLContentSink::HandleEndElement(xul:gripper)
mDocumentURI:
jar:resource:///chrome/toolkit.jar!/content/global/bindings/scrollbar.xml
nsXBLContentSink::HandleEndElement(content)
mDocumentURI:
jar:resource:///chrome/toolkit.jar!/content/global/bindings/scrollbar.xml
nsXBLContentSink::HandleEndElement(binding)
mDocumentURI:
jar:resource:///chrome/toolkit.jar!/content/global/bindings/scrollbar.xml
About to call mBinding->Initialize() with mBinding=0x00000000

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.. We should probably fix things here to handle the error in addition to
fixing the attr code....

Comment 3

[Philip K. Warren]

This seems to be why the problem is occurring:

nsAttrAndChildArray::SetAttr: &ATTRS(mImpl)[0].mName = 0x20e4c180
nsAttrAndChildArray::SetAttr: &ATTRS(mImpl)[0].mValue = 0x20e4c184
nsAttrAndChildArray::SetAttr: &ATTRS(mImpl)[1].mName = 0x20e4c184
nsAttrAndChildArray::SetAttr: &ATTRS(mImpl)[1].mValue = 0x20e4c188

So the first time through SetAttr, the name and value are set properly. The
second time through, the name offset is the same as the first's value offset, so
the value of the first attribute is overwritten with the name of the second
attribute.

Comment 4

[Boris Zbarsky [:bzbarsky]]

pkw, "unsigned long" and "void*" are both 64 bits on AIX, right?

sicking, should ATTRSIZE be coming out with 1 or 2?

Comment 5

[Philip K. Warren]

pkw@ut:~$ cc test.c -o test
pkw@ut:~$ ./test 
sizeof(unsigned long)=4
sizeof(void *)=4
pkw@ut:~$ cc -q64 test.c -o test64
pkw@ut:~$ ./test64 
sizeof(unsigned long)=8
sizeof(void *)=8

So they are both 4 bytes when compiled in 32-bit mode, and 8 bytes when compiled
in 64-bit mode. I haven't built this yet in 64-bit mode - this is failing with a
32-bit build.

Comment 6

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

ATTRSIZE should come out as 2. However I think this is an operator-precedence
problem, still looking into the matter.

Comment 7

[Philip K. Warren]

Attachment: Patch v1

This seems to fix the problem. I am not sure if this is a compiler error or
not. I need to put together a small testcase to send to the compiler folks to
look over.

Comment 8

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

Comment on attachment 139218 [details] [diff] [review]
Patch v1

Feel free to add a comment since this was pretty nasty to find. And thanks for
all the help finding it!!

Comment 9

[tor]

Comment on attachment 139218 [details] [diff] [review]
Patch v1

Yes, a comment explaining this and a pointer to the bug would
be invaluable to the next unsuspecting victim to visit this
code.  sr=tor

Comment 10

[Philip K. Warren]

Checked in (with a comment explaining the change and a pointer back to this
bug). I will restart the AIX tinderbox to pick up the change immediately.
Hopefully it will go green now.

Checking in nsAttrAndChildArray.cpp;
/cvsroot/mozilla/content/base/src/nsAttrAndChildArray.cpp,v  <-- 
nsAttrAndChildArray.cpp
new revision: 3.3; previous revision: 3.2
done

Comment 11

[Philip K. Warren]

When testing the latest Thunderbird trunk build, I discovered a similar error in
nsAttrAndChildArray::AttrAt:

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsAttrAndChildArray.h#95

This is the same usage which triggered the AIX bug before that we resolved in
the ATTRS define. Should the ATTRS define be moved to the header file and the
usage at this location be switched to use it, or should I just modify the line:

return &NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer)[aPos].mValue;

to read:

return &NS_REINTERPRET_CAST(InternalAttr*, &(mImpl->mBuffer[0]))[aPos].mValue;

Comment 12

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

Moving the ATTRS definition to the headerfile and using it in AttrAt sounds like
the best alternative.

Comment 13

[Philip K. Warren]

Attachment: Patch v2

Changes AttrAt to use the fixed ATTRS define as well. This was causing a crash
on startup in Thunderbird.

Comment 14

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

tor pointed out that it's not really a good idea to have a macro with that name
in a .h file (which is why i put it in the .cpp file in the first place).

Anyhow, this inlined function is going away very soon anyway (it's being moved
into the .cpp file) so the problem will automatically be taken care of then. For
now just change the code to do:

return &NS_REINTERPRET_CAST(InternalAttr*, &(mImpl->mBuffer[0]))[aPos].mValue;

r=me sr=tor on that.

Comment 15

[Philip K. Warren]

Fixed.

Checking in nsAttrAndChildArray.h;
/cvsroot/mozilla/content/base/src/nsAttrAndChildArray.h,v  <-- 
nsAttrAndChildArray.h
new revision: 3.3; previous revision: 3.2
done

Comment 16

[tor]

Comment on attachment 139614 [details] [diff] [review]
Patch v2

Marking to remove from request queue.