Closed Bug 430581 Opened 12 years ago Closed 11 years ago
String and ns String _external has different size on x86 _64
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
Nominating for blocking because this will affect 64 bit extensions. Marco, can you produce a patch?
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-
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 #322005 - Flags: review? → review?(benjamin)
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?
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-
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 approval22.214.171.124 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: approval126.96.36.199?
Attachment #322005 - Flags: approval188.8.131.52? → approval184.108.40.206?
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?
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.
Marco is working on fixing his Hg access (he should have access already).
Status: NEW → ASSIGNED
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 220.127.116.11.
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]
Comment on attachment 322005 [details] [diff] [review] Use PR_STATIC_ASSERT to check size at compile time Approved for 18.104.22.168. Please land in CVS. a=ss
Attachment #322005 - Flags: approval22.214.171.124? → approval126.96.36.199+
Marco, if you want this in 188.8.131.52, we need it landed asap.
You need to log in before you can comment on or make changes to this bug.