Closed Bug 430581 Opened 12 years ago Closed 11 years ago

nsString and nsString_external has different size on x86_64

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mpgritti, Assigned: mpgritti)

Details

(Keywords: fixed1.9.0.2)

Attachments

(1 file, 2 obsolete files)

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
Version: unspecified → Trunk
Nominating for blocking because this will affect 64 bit extensions.

Marco, can you produce a patch?
Flags: blocking1.9?
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?
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.
-'ing per comment 3.
Flags: blocking1.9? → blocking1.9-
Attached patch Fix (obsolete) — Splinter Review
Attachment #318839 - Attachment description: Fix (not tested on i386 yet) → Fix
Attachment #318839 - Flags: review?(benjamin)
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.
Attachment #318839 - Flags: review?(benjamin) → review-
I'm not sure if nsSubstring.cpp is the best place for the PR_STATIC_ASSERT, better ideas are welcome.
Attachment #318839 - Attachment is obsolete: true
Attachment #322003 - Flags: review?(benjamin)
Attachment #322003 - Attachment is obsolete: true
Attachment #322003 - Flags: review?(benjamin)
Attachment #322005 - Flags: review? → review?(benjamin)
Attachment #322005 - Flags: review?(benjamin) → review+
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.
Attachment #322005 - Flags: approval1.9?
Whiteboard: [RC2?]
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.
Attachment #322005 - Flags: approval1.9? → approval1.9-
Whiteboard: [RC2?] → [RC2-]
Just to clarify, I tested it on x86 before submitting it for review.
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 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.
Attachment #322005 - Flags: approval1.9.0.1?
Attachment #322005 - Flags: approval1.9.0.1? → approval1.9.0.2?
Can we get this landed on m-c (resolve it as FIXED when that's done) before approving for 1.9.0.x?
What does m-c stand for?
mozilla-central
Ok. I'll try to get a mozilla-central account.
(In reply to comment #17)
> Ok. I'll try to get a mozilla-central account.

Alternatively, someone else can check this in for you.
Keywords: checkin-needed
Assignee: nobody → mpgritti
Marco is working on fixing his Hg access (he should have access already).
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [RC2-] → [needs mozilla central landing before 1.9 approval]
We need to get this landed in mozilla-central soon if you want it in 1.9.0.2.
Landed on mozilla-central. Sorry for the delay and thanks for reminding me about it.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs mozilla central landing before 1.9 approval] → [needs baking]
Whiteboard: [needs baking]
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
Attachment #322005 - Flags: approval1.9.0.2? → approval1.9.0.2+
Marco, if you want this in 1.9.0.2, we need it landed asap.
Keywords: fixed1.9.0.2
You need to log in before you can comment on or make changes to this bug.