693240 - Lots of assertion with SPARC after s/PRBool/bool/

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ginn Chen]

I got lots of assertion on SPARC after Bug 675553.

Most of the assertions are:

###!!! ASSERTION: null ptr: 'aOldTarget != nsnull', file /export/home/ginn/work/mozilla-central/rdf/base/src/nsInMemoryDataSource.cpp, line 1576
###!!! ASSERTION: null ptr: 'aOldTarget != nsnull', file /export/home/ginn/work/mozilla-central/rdf/base/src/nsInMemoryDataSource.cpp, line 1576
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIRDFDataSource.Change]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: file:///export/home/ginn/work/mozilla-central/debug/dist/bin/components/nsHandlerService.js :: HS__setTarget :: line 1265"  data: no]
************************************************************

And some others like
###!!! ASSERTION: Oops!  You're asking for a weak reference to an object that doesn't support that.: 'factoryPtr', file /export/home/ginn/work/mozilla-central/debug/xpcom/build/nsWeakReference.cpp, line 109
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: resource://gre/modules/XPCOMUtils.jsm :: XPCU_serviceLambda :: line 232"  data: no]

or

###!!! ASSERTION: Won't check if this is chrome, you want to set aWantsUntrusted to PR_FALSE or make the aWantsUntrusted explicit by making aOptionalArgc non-zero.: '!aWantsUntrusted || aOptionalArgc > 1', file /export/home/ginn/work/mozilla-central/content/base/src/nsGenericElement.cpp, line 1078
JavaScript strict warning: file:///export/home/ginn/work/mozilla-central/debug/dist/bin/components/nsSessionStore.js, line 4360: reference to undefined property aBrowser.__SS_restoreState

Comment 1

[Ginn Chen]

Looks like we should revert the change for
xpcom/reflect/xptcall/src/md/unix
at least for big endian machines.

Or perhaps s/PRBool/uint32

mwu, suggestions?

Comment 2

[Michael Wu [:mwu]]

I don't know which files are built for your platform, but if it's anything like xptcinvoke_sparc_solaris.cpp, I suspect that 

case nsXPTType::T_BOOL   : *((bool*) l_d) = l_s->val.b;           break;

should be changed to

case nsXPTType::T_BOOL   : *((uint32*) l_d) = l_s->val.b;           break;

in order to match the handling of other data types that are less than 4 bytes.

Comment 3

[Ginn Chen]

Attachment: patch

I hope I didn't miss a file.

Fix xptcstubs_sparcv9_solaris.cpp according to xptcstubs_sparc64_openbsd.cpp

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 567320 [details] [diff] [review]
patch

I'm not comfortable enough with this code to review.

However, in the xptcstubs_netbsd_m68k.cpp case, I think we may be able to get away with a bool there as long as the + 3 offset remains there. This would probably avoid a cast and eliminate an instruction or two.

Comment 5

[Benjamin Smedberg]

Comment on attachment 567320 [details] [diff] [review]
patch

glandium said he would look at this, since I don't know the assembly and he knows at least some of the architectures here.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 567320 [details] [diff] [review]
patch

Review of attachment 567320 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/reflect/xptcall/src/md/unix/xptcstubs_linux_m68k.cpp
@@ +94,5 @@
>              case nsXPTType::T_U32    : dp->val.u32 = *((PRUint32*)ap);       break;
>              case nsXPTType::T_U64    : dp->val.u64 = *((PRUint64*)ap); ap++; break;
>              case nsXPTType::T_FLOAT  : dp->val.f   = *((float*)   ap);       break;
>              case nsXPTType::T_DOUBLE : dp->val.d   = *((double*)  ap); ap++; break;
> +            case nsXPTType::T_BOOL   : dp->val.b   = *(((char*)   ap) + 3);  break;

I understand this is the style in this file, but it might be better to do like on other architectures, using *((PRUint32*)ap)

Other than that, r=me.

Comment 7

[Ginn Chen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee987590740

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/2ee987590740