225952 - *getter_Copies abuse

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[[e-mail]]

Two files abuse getter_Copies by assigning into *getter_Copies. At the very
least they should be using Adopt, but the actual uses need rewriting anyway.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

when touching objectframe, you could also change this:
978 nsObjectFrame::MakeAbsoluteURL(nsIURI* *aFullURI, 
979                               nsString aSrc,

to use const nsAString& instead of nsString

Comment 2

[[e-mail]]

Attachment: Fix nsObjectFrame.cpp

Comment 3

[[e-mail]]

Attachment: Fix gtkmozembed2.cpp

Comment 4

[[e-mail]]

Comment on attachment 135710 [details] [diff] [review]
Fix gtkmozembed2.cpp

>-  if (embedPrivate->mURI.Length()) {
>-    *getter_Copies(embedString) = NEW_TOOLKIT_STRING(embedPrivate->mURI);
>-    retval = strdup(embedString);
>-  }
>+  if (!embedPrivate->mURI.IsEmpty())
>+    retval = NEW_TOOLKIT_STRING(embedPrivate->mURI);

unfortunately this mismatches the allocators... NEW_TOOLKIT_STRING is defined
as either ToNewUTF8String or ToNewCString (see lxr). while i had a hard time
finding callers in the tree of any of the functions you're changing, it does
seem that they want to free the string using g_free. which means you must
allocate it using g_strdup.

so, i would recommend doing something like this...

#define NEW_TOOLKIT_STRING(x) g_strdup(NS_ConvertUTF16toUTF8(x).get())

and accordingly for the ASCII variant. not pretty, but it's correct. :/

Comment 5

[[e-mail]]

Attachment: Better fix for nsObjectFrame.cpp

Comment 6

[[e-mail]]

Attachment: Better fix for gtkmozembed2.cpp

Comment 7

[[e-mail]]

Attachment: Real better fix for gtkmozembed2.cpp

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135714 [details] [diff] [review]
Better fix for nsObjectFrame.cpp

r+sr=bzbarsky.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 135717 [details] [diff] [review]
Real better fix for gtkmozembed2.cpp

sr=jst

Comment 10

[Christopher Blizzard (:blizzard)]

Comment on attachment 135717 [details] [diff] [review]
Real better fix for gtkmozembed2.cpp

This changes the allocator used for the return values for those functions. 
Please use strdup, if anything.

Comment 11

[[e-mail]]

Attachment: Updated for review comment

Comment 12

[[e-mail]]

Comment on attachment 136723 [details] [diff] [review]
Updated for review comment

Assuming jst's sr still applies, re-requesting review.

Comment 13

[Marco Pesenti Gritti]

The changes in gtkmozembed2 collides with 205425 (it ports gtkmozembed to use
nsEmbedString). I can deal it with it but if we can get a review of that patch
soon maybe it's worth to avoid the conflict.

Comment 14

[[e-mail]]

Attachment: Updated for bitrot

Transferring jst's sr.

Comment 15

[[e-mail]]

Fix checked in.