Checkin for Bug 195350 causes AIX build to segfault on startup

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: Philip K. Warren, Unassigned)

Tracking

Trunk
Other
AIX
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 139193 [details]
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
Hmm.. We should probably fix things here to handle the error in addition to
fixing the attr code....
(Reporter)

Comment 3

14 years ago
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.
pkw, "unsigned long" and "void*" are both 64 bits on AIX, right?

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

Comment 5

14 years ago
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.
ATTRSIZE should come out as 2. However I think this is an operator-precedence
problem, still looking into the matter.
(Reporter)

Comment 7

14 years ago
Created attachment 139218 [details] [diff] [review]
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.
(Reporter)

Updated

14 years ago
Attachment #139218 - Flags: superreview?(tor)
Attachment #139218 - Flags: review?(bugmail)
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!!
Attachment #139218 - Flags: review?(bugmail) → review+

Comment 9

14 years ago
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
Attachment #139218 - Flags: superreview?(tor) → superreview+
(Reporter)

Comment 10

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Reporter)

Updated

14 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 11

14 years ago
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;
Moving the ATTRS definition to the headerfile and using it in AttrAt sounds like
the best alternative.
(Reporter)

Comment 13

14 years ago
Created attachment 139614 [details] [diff] [review]
Patch v2

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

Updated

14 years ago
Attachment #139614 - Flags: superreview?(tor)
Attachment #139614 - Flags: review?(bugmail)
Attachment #139614 - Flags: review?(bugmail) → review+
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.
(Reporter)

Comment 15

14 years ago
Fixed.

Checking in nsAttrAndChildArray.h;
/cvsroot/mozilla/content/base/src/nsAttrAndChildArray.h,v  <-- 
nsAttrAndChildArray.h
new revision: 3.3; previous revision: 3.2
done
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 16

14 years ago
Comment on attachment 139614 [details] [diff] [review]
Patch v2

Marking to remove from request queue.
Attachment #139614 - Flags: superreview?(tor) → superreview+

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.