740688 - Use uintptr_t instead of PRUword, and intptr_t instead of PRWord

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jeff Walden [:Waldo]]

Per NSPR, "A PRWord is an integer that is the same size as a void*", and mutatis mutandis for PRUword.  This is straightforwardly the same as intptr_t and uintptr_t.  We should use those *standard* types (found in #include "mozilla/StandardInteger.h") rather than the PR{Uw,W}ord types -- which, further, are accompanied by this lovely comment:

502 ** WARNING: The undocumented data types PRWord and PRUword are
503 ** only used in the garbage collection and arena code.  Do not
504 ** use PRWord and PRUword in new code.

Note that uses in nspr/pr shouldn't be changed -- NSPR is its own project, we don't modify it, and they use obfuscatory types more readily than we do these days.  The ipc/chromium file probably should have its code for these types removed -- they're not used in the code we imported, and our policy (for better or worse) is that we don't care that we've forked that code.  Finally, security/nss shouldn't be changed, because NSS is also its own project.  (It probably shouldn't be using PRWord either, but at the very least that should be changed in a separate bug.)

Comment 1

[Mark Capella [:capella]]

I don't see anyone assigned to this. If you have no objections I'll work on it.

I scanned for PRWord and PRUword type keywords, and removing the files in folders nspr/pr and security/nss, I wind up the following list of affected files:

/ipc/chromium/src/base/third_party/nspr/prtypes.h

/caps/include/nsScriptSecurityManager.h
/caps/src/nsScriptSecurityManager.cpp

/content/xslt/src/xpath/txMozillaXPathTreeWalker.cpp

/dom/plugins/base/nsNPAPIPluginStreamListener.cpp

/layout/base/nsDisplayList.cpp
/layout/base/nsDisplayList.h
/layout/base/nsPresArena.cpp
/layout/base/nsPresArena.h
/layout/generic/nsGfxScrollFrame.cpp
/layout/style/nsRuleData.cpp
/layout/style/nsRuleNode.h

/js/xpconnect/src/XPCWrappedNative.cpp
/js/xpconnect/src/xpcprivate.h

/xpcom/base/nsCycleCollector.cpp
/xpcom/base/nscore.h
/xpcom/glue/nsVoidArray.h
/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_mips.cpp
/xpcom/reflect/xptcall/src/md/unix/xptcstubs_mips.cpp

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

Well I had some abndwitdh available so I went and coded / built / tested this attached patch locally. It all looks good.

I tried to remove the PR{Uw,W}ord type definitions from the /ipc/chromium/src/base/third_party/nspr/prtypes.h file, but that caused a build failure centered around C:\Users\Master\mozilla-central\nsprpub\pr\include\obsolete\protypes.h so I left it in place.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 612351 [details] [diff] [review]
Patch (v1)

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

::: caps/include/nsScriptSecurityManager.h
@@ +146,5 @@
>  
>  // Property Policy
>  union SecurityLevel
>  {
> +    intptr_t   level;

Preserve alignment here, since the old code did.

::: caps/src/nsScriptSecurityManager.cpp
@@ +3341,1 @@
>                   "This may cause a security failure with the SecurityLevel union.");

I bet this predated MOZ_STATIC_ASSERT.  Switch to that here?

::: js/xpconnect/src/xpcprivate.h
@@ +2850,5 @@
>      XPCNativeSet*                mSet;
>      JSObject*                    mFlatJSObject;
>      XPCNativeScriptableInfo*     mScriptableInfo;
>      XPCWrappedNativeTearOffChunk mFirstChunk;
> +    intptr_t                       mWrapperWord;

Alignment.

::: layout/style/nsRuleData.cpp
@@ +48,5 @@
>    // offsets.
> +  MOZ_STATIC_ASSERT(sizeof(uintptr_t) == sizeof(size_t),
> +                    "expect uintptr_t and size_t to be the same size");
> +  MOZ_STATIC_ASSERT(uintptr_t(-1) > uintptr_t(0),
> +                    "expect uintptr_t to be unsigned");

Arguably we could just remove this, since uintptr_t is a standard type, and if it were broken a whole lot of other things would be broken besides this.  But it doesn't really hurt, either.  And I guess we're doing the same thing for size_t below, so meh.

::: xpcom/base/nscore.h
@@ +394,5 @@
>   */
>  
> +#define NS_PTR_TO_INT32(x)  ((PRInt32)  (intptr_t) (x))
> +#define NS_PTR_TO_UINT32(x) ((PRUint32) (intptr_t) (x))
> +#define NS_INT32_TO_PTR(x)  ((void *)   (intptr_t) (x))

Ugh these macros are so anachronistic.  I guess we shouldn't do anything about them here, tho.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v2)

Nits addressed ... builds and tests locally.

Comment 5

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 613056
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=2a51ac9edfb2
Try run started, revision 2a51ac9edfb2. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=2a51ac9edfb2

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v3)

Comment 7

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 613824
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=d88dd126c9bc
Try run started, revision d88dd126c9bc. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d88dd126c9bc

Comment 8

[Mozilla RelEng Bot]

Try run for d88dd126c9bc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d88dd126c9bc
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d88dd126c9bc

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 613824 [details] [diff] [review]
Patch (v3)

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

Looks good modulo one super-nit.  I'll fix that and land this, probably tomorrow.  Thanks!

::: js/xpconnect/src/xpcprivate.h
@@ +48,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/StandardInteger.h"

This should be before Util.h to be alphabetical.

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/622e0d1cc986

Good stuff!  More NSPR craziness dies a painful death.

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/622e0d1cc986