430581 - nsString and nsString_external has different size on x86_64

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Marco Pesenti Gritti]

It has been fixed on i386 by bug 390849, but it's still different on x86_64. 

Because of this, using nsIDocument in an extensions results in hard to debug crashes. For reference https://bugzilla.redhat.com/show_bug.cgi?id=441643

On x86_64:

(gdb) p sizeof ( nsString )
$7 = 16
(gdb) p sizeof ( nsString_external )
$8 = 24

On i386:
(gdb) p sizeof(nsString)
$1 = 12
(gdb) p sizeof(nsString_external)
$2 = 12

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Nominating for blocking because this will affect 64 bit extensions.

Marco, can you produce a patch?

Comment 2

[Damon Sicore (:damons)]

Why is this just coming up now?  Are you suggesting blocking because we can't change this in a dot release?

re-nom when answered?

Comment 3

[Benjamin Smedberg]

I don't think this should block. The frozen string API is working correctly... the struct is big enough, it just happens to be too big. This doesn't break the API directly: it only breaks people who use internal pseudo-interfaces.

Comment 4

[Damon Sicore (:damons)]

-'ing per comment 3.

Comment 5

[Marco Pesenti Gritti]

Attachment: Fix

Comment 6

[Benjamin Smedberg]

Comment on attachment 318839 [details] [diff] [review]
Fix

I think this is ok, but I'd like a testcase somewhere which asserts that sizeof(nsStringContainer_base) == sizeof(nsSubstring)... I think PR_STATIC_ASSERT can be used to make a compile-time testcase.

Comment 7

[Marco Pesenti Gritti]

Attachment: Use PR_STATIC_ASSERT to check size at compile time

I'm not sure if nsSubstring.cpp is the best place for the PR_STATIC_ASSERT, better ideas are welcome.

Comment 8

[Marco Pesenti Gritti]

Attachment: Use PR_STATIC_ASSERT to check size at compile time

Comment 9

[Benjamin Smedberg]

Comment on attachment 322005 [details] [diff] [review]
Use PR_STATIC_ASSERT to check size at compile time

This is very low-risk and I think we should take it so we can publish the XULRunner 1.9 SDK with the fix.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 322005 [details] [diff] [review]
Use PR_STATIC_ASSERT to check size at compile time

Per discussion with :bs, not necessary for 3.0, and tbh comments like "untested on x86" make me think that we might want to try it on other platforms a wee bit as well.  3.0.1 will be a fine time to release the 1.9 SDK.

Comment 11

[Marco Pesenti Gritti]

Just to clarify, I tested it on x86 before submitting it for review.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Hey Marco: if this is to be considered for 3.0.1 we'll need:

 - complete assessment of risk, and what testing was done to mitigate
 - an approval1.9.0.1 request

Comment 13

[Marco Pesenti Gritti]

Comment on attachment 322005 [details] [diff] [review]
Use PR_STATIC_ASSERT to check size at compile time

As far as I can tell this is very low risk (Benjamin agrees with that in comment #9). If there are issues the PR_STATIC_ASSERT should catch them. I tested it both on x86 and x86_64.

Comment 14

[Samuel Sidler (old account; do not CC)]

Can we get this landed on m-c (resolve it as FIXED when that's done) before approving for 1.9.0.x?

Comment 15

[Marco Pesenti Gritti]

What does m-c stand for?

Comment 16

[Samuel Sidler (old account; do not CC)]

mozilla-central

Comment 17

[Marco Pesenti Gritti]

Ok. I'll try to get a mozilla-central account.

Comment 18

[Samuel Sidler (old account; do not CC)]

(In reply to comment #17)
> Ok. I'll try to get a mozilla-central account.

Alternatively, someone else can check this in for you.

Comment 19

[Reed Loden [:reed]]

Marco is working on fixing his Hg access (he should have access already).

Comment 20

[Samuel Sidler (old account; do not CC)]

We need to get this landed in mozilla-central soon if you want it in 1.9.0.2.

Comment 21

[Marco Pesenti Gritti]

Landed on mozilla-central. Sorry for the delay and thanks for reminding me about it.

Comment 22

[Samuel Sidler (old account; do not CC)]

Comment on attachment 322005 [details] [diff] [review]
Use PR_STATIC_ASSERT to check size at compile time

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 23

[Samuel Sidler (old account; do not CC)]

Marco, if you want this in 1.9.0.2, we need it landed asap.