178499 - AIX: xptcinvoke passes invalid boolean values in 64-bit

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Philip K. Warren]

When Mozilla is compiled in 64-bit mode for AIX, there are several of the
following error messages printed to the screen:

###!!! ASSERTION: AddObserver: trying weak object that doesnt support
nsIWeakReference: 'weakRefFactory', file
/home/pkw/builds/trunk/mozilla/xpcom/ds/nsObserverList.cpp, line 79
Break: at file /home/pkw/builds/trunk/mozilla/xpcom/ds/nsObserverList.cpp, line 79
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsIObserverService.addObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)" 
location: "JS frame ::
file:///home/pkw/builds/trunk/mozilla/obj-debug64/dist/bin/components/nsUpdateNotifier.js
:: anonymous :: line 67"  data: no]
************************************************************

In addition, the View->Text Zoom menu shows duplicate entries, the designated
home page is not loaded on startup, and selecting a sidebar tab causes the
browser to enter into an infinite loop.

I traced the origin of one of the addObserver calls (originating in
xpfe/components/updates/src/nsUpdateNotifier.js). This is how it calls addObserver:

observerService.addObserver(this, "domwindowopened", false);

The third parameter to the addObserver function
(http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsObserverService.cpp#152) is
a PRBool which specifies whether the observer object supports weak references.
The error messages above would indicate that the PRBool was set to true, while
obviously the call to addObserver in nsUpdateNotifier.js passes false. I printed
the value of the PRBool ownsWeak when the errors above occurred, and found that
it was set to a very large negative number.

The error which causes PRBools to not be passed correctly in 64-bit is in the
xptcinvoke invoke_copy_to_stack function, specifically the following:

*((PRBool*) l_d) = l_s->val.b;

This assigns the value of the boolean into the top 32-bits of the 64-bit number
(PRBools are defined as PRIntn (int)). I have a patch to fix this problem on AIX.

Comment 1

[Philip K. Warren]

I have two patches - one to TestXPTCInvoke which adds a new test using booleans,
and one which fixes the 64-bit assembly on AIX. I will add them to this bug
tomorrow.

Comment 2

[Philip K. Warren]

Attachment: New TestXPTCInvoke boolean test

Comment 3

[Philip K. Warren]

Attachment: AIX 64-bit xptcinvoke fix

This fixes the 64-bit builds on AIX. This includes a lot of whitespace changes,
as well as changes to simplify xptcstubs, so it looks much more complicated
than it is. If the reviewers would like me to submit a small patch first I'd be
happy to do so.

Comment 4

[Philip K. Warren]

Attachment: TestXPTCInvoke before patch (includes boolean test)

Comment 5

[Philip K. Warren]

Attachment: TestXPTCInvoke after patch (includes boolean test)

Comment 6

[Philip K. Warren]

The results of TestXPTCInvoke before/after the patch clearly show the problems
with booleans in 64-bit on AIX.

Before:
1 + 0 + 1 + 0 + 1 + 0 + 1 + 0 + 1 + 0 = 73055486

After:
1 + 0 + 1 + 0 + 1 + 0 + 1 + 0 + 1 + 0 = 5

Looking for a review on the AIX 64-bit xptcinvoke fix, and would like to check
in the new test to XPTCInvoke as well, since I believe a similar fix will be
needed on Solaris 64-bit (see http://bugzilla.mozilla.org/show_bug.cgi?id=69815#c8).

Comment 7

[Philip K. Warren]

Attachment: AIX 64-bit xptcinvoke fix (diff -uw)

cvs diff -uw which cleans up the patch significantly. The last patch included
too many whitespace changes which made it hard to read.

Comment 8

[David Bradley]

Should we be using NS_STATIC_CAST so we're explicit on what we're trying to do.
But that's a minor nit. A bigger issue, won't the casting to PRUint64 create
problems for negative values? I think that's why the code originally was casting
the left side to PRInt64 and then PRUint64, although the latter I don't think is
needed.

Comment 9

[Philip K. Warren]

I have tested that this does not cause any problems with passing negative 
values. I changed them all to be PRUint64 for simplicity - I believe the same 
is done in the invoke_copy_to_stack on Alpha. We should probably improve the 
tests in TestXPTCInvoke to cover passing negative numbers as well. The current 
tests are pretty basic, and that's part of the reason why we were not alerted 
to the problem in passing boolean values in our initial port to 64-bit.

NS_STATIC_CAST would probably be a good idea.

Comment 10

[David Bradley]

I was wrong, the only problem is if the target was smaller than the source which
it isn't. Otherwise it's a bitwise assignment so it will do what is desired.

Comment 11

[Jim Dunn]

Comment on attachment 105592 [details] [diff] [review]
AIX 64-bit xptcinvoke fix (diff -uw)

r=[e-mail]

Comment 12

[John Bandhauer]

[I didn't actually mean to mark the bug as reviewed by me - just trying to set
the bugzilla flag to reflect jdunn's review. Bugzilla is getting annoyingly
smart these days.]

I don't actually have any serious problems with this patch . I am bothered that
it is full of meaningless reformating that obscures the *real* changes; who
cares if comments are in '/**/' or '//' ? And, do I see tabs creeping in?

+#define ASSIGN_GPR_VALUE(member,type) \
+if(iCount < PARAM_GPR_COUNT) \
+    dp->val.member = (type) gprData[iCount++]; \
+else \
+    dp->val.member = (type) *ap++;

FWIW, I wouldn't normally mark this as r=jband without demanding that this be
rewritten to produce a single statement. It should either be bracketted by
do{...}while(0) or be rewritten as an '?:' assignment. Note that the current
macro ends in a ';' and its uses are followed with ';' - i.e. you are already
getting the macro's use slightly wrong.

But, when I owned xptcall my attitude was to let capable contributors do pretty
much whatever they wanted in these files as long as the code actually functions
correctly on the given platform.

I am no longer an 'sr' and will send email on your behalf requesting sr to a
couple of people.

Comment 13

[Philip K. Warren]

Attachment: Simpler patch

Remove all of the whitespace changes. This should be a more acceptable patch.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 105876 [details] [diff] [review]
Simpler patch

sr=jst

Comment 15

[Jim Dunn]

Comment on attachment 105876 [details] [diff] [review]
Simpler patch

r=jdunn

Comment 16

[Philip K. Warren]

Checked in. (xptcinvoke_ppc_aix64.cpp v1.2)

Comment 17

[Phil Schwartau]

check-in Verified -