Closed Bug 450881 Opened 11 years ago Closed 6 years ago

nsCOMArray: Helpers for XPIDL-based arrays? [array, size_is(count)] in/out nsIFoo fooObjects]

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: WeirdAl, Assigned: jcranmer)

Details

Attachments

(1 file)

<WeirdAl> I have an interface method written like this: |void handleArray(in PRUint32 count, [array, size_is(count)] nsIFoo fooObjects);| and I have a nsCOMArray in the caller. I'm wondering if I should change the interface to take a nsIArray, or if I should come up with a generic conversion from nsCOMArray<nsIFoo> to nsIFoo**, or if I should do something else
<NeilAway> it should be possible to write a new T** nsCOMArray<T>::Elements() method that returns a flat array that you can pass as an IDL parameter
<WeirdAl> your thinking parallels mine (new method) - two questions. (1) Cleaning up the memory afterwards. (2) Is there enough demand to justify adding it to nsCOMArray? I see no use cases in current C++ code.
<NeilAway> there's no memory to clean up in the Elements() case, you'd just write nsCOMArray<T> tArray; /* ... */ doFoo(T.Elements(), T.Count());
<WeirdAl> I don't quite follow. How would we create the T** object in the first place?
<WeirdAl> (Mozilla's C++ templates have spoiled me; it's very hard for me to write raw C++)
<NeilAway> it should be possible to access the raw array,
<NeilAway> possibly via a helper method on nsVoidArray
<NeilAway> template<class T> class nsCOMArray : public nsCOMArray_base { ... T** Elements() { return reinterpret_cast<T**>(mArray.Elements()); } ... }
<NeilAway> class nsVoidArray { ... void **Elements() { return mImpl->mArray; } ... }
<NeilAway> hmm, actually mImpl could be null, I'd have to write { return mImpl ? mImpl->mArray : nsnull; }
<WeirdAl> would this implementation also be useful in the outparam case?
<NeilAway> no, a helper class would work best for that
* WeirdAl wouldn't think so - you'd need a constructor
<NeilAway> no actually, outparam is hard, because you have the data and the length
<WeirdAl> so it's a two-argument helper
<NeilAway> no, I mean you'd have to write another method on nsCOMArray to "adopt" an outparam
<WeirdAl> ok, you and I are actually thinking on the same track
<WeirdAl> now, here's the kicker: is there enough value in these ideas to warrant adding them to mozilla-central? I saw practically no demand for in params, and I haven't looked at out params
<NeilAway> Adopt(T** data, PRInt32 count) { Clear(); SetSize(count); memcpy(Elements(), data, count * sizeof(T*)); NS_Free(data); } perhaps
<WeirdAl> I haven't filed a bug, because I'm not sure it's needed
<NeilAway> I'd start by looking though all the interfaces to see how many of them pass arrays of pointers
<WeirdAl> I did that for in params - all callers were JS
<WeirdAl> mozilla1.8, I did find one C++-based caller
<NeilAway> numbers aren't on your side then :-(
<WeirdAl> I do see six out nsI* methods in IDL that look promising, though, including nsIXTFElement... XForms guys would love that converter if they don't already have it
<NeilAway> maybe you can get them to push it

Code like this would be of use at Skyfire, since I have a XPIDL interface I've written to take a XPIDL array of objects - and both C++ and JS callers.

mozilla.org code would find it less useful, based on a quick search:
http://mxr.mozilla.org/mozilla-central/search?string=size_is&find=\.idl%24&findi=\.idl%24&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

With the help Neil provided above, I can write a patch if desired.
Any helpers we write may need an extra argument (PRBool AddRefElements or PRBool ReleaseElements).  Reason:  nsCOMArray::~nsCOMArray releases references, so when the array goes out of scope, we risk destroying the array elements we're trying to send out.
Strictly speaking, the move constructor/assignment operators don't need to be part of this patch, but they were useful for other work I was doing in comm-central, so I threw them in here as well.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #8362735 - Flags: review?(nfroyd)
Comment on attachment 8362735 [details] [diff] [review]
Add rvalue methods, Adopt, and Forget to nsCOMArray

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

r=me with the miscellaneous fixes below.

::: xpcom/glue/nsCOMArray.cpp
@@ +279,5 @@
>  
>      return n;
>  }
> +    
> +void

Nit: whitespace above "void" here.

@@ +284,5 @@
> +nsCOMArray_base::Adopt(nsISupports** aElements, uint32_t aSize)
> +{
> +    Clear();
> +    // Don't use nsCOMArray::AppendElements at here--we're stealing the
> +    // reference counts from the array.

This seems like an odd comment, given that we're in nsCOMArray_base at this point.  Delete it please.

@@ +297,5 @@
> +{
> +    uint32_t length = Length();
> +    nsISupports** array = static_cast<nsISupports**>(
> +        moz_xmalloc(sizeof(nsISupports*) * length));
> +    memmove(array, Elements(), sizeof(nsISupports*) * length);

You could pull out the sizeof()*length into a local for these two uses; you shouldn't have to wrap the |array| declaration then.  Same # of lines, but I think it reads a little clearer.

::: xpcom/glue/nsCOMArray.h
@@ +404,5 @@
> +     * nsCOMArray<nsISomeInterface> array;
> +     * nsISomeInterface** elements;
> +     * uint32_t length;
> +     * ptr->GetSomeArray(&elements, &length);
> +     * array.Adopt(elements, length);

I think it would be good to note here that |elements| is freed.

@@ +412,5 @@
> +            aSize);
> +    }
> +
> +    /**
> +     * Export the contents of this array to an XPIDL outparam.

Please add a note that the array is Clear()'d after this operation.
Attachment #8362735 - Flags: review?(nfroyd) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e057ecce0007 because of Windows build bustage:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33363925&tree=Mozilla-Inbound

Processing config: 
Processing plugin dlls: "c:\mozilla-build\nsis-2.46u\Plugins\*.dll"
webapprt.cpp
webapprt-stub.exe
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\config\rules.mk:744:0$ c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/expandlibs_exec.py --depend .deps/webapprt-stub.exe.pp --target webapprt-stub.exe --uselist -- link -NOLOGO -OUT:webapprt-stub.exe -PDB:webapprt-stub.pdb -ENTRY:wmainCRTStartup -SUBSYSTEM:WINDOWS -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH  -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF   /HEAP:0x40000  webapprt.obj ./module.res c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/dist/lib/xpcomglue_staticruntime.lib   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib shell32.lib
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\config\rules.mk:744:0: command 'c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/_virtualenv/Scripts/python.exe c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/config/expandlibs_exec.py --depend .deps/webapprt-stub.exe.pp --target webapprt-stub.exe --uselist -- link -NOLOGO -OUT:webapprt-stub.exe -PDB:webapprt-stub.pdb -ENTRY:wmainCRTStartup -SUBSYSTEM:WINDOWS -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH  -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF   /HEAP:0x40000  webapprt.obj ./module.res c:/builds/moz2_slave/m-in-w32-000000000000000000000/build/obj-firefox/dist/lib/xpcomglue_staticruntime.lib   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib shell32.lib' failed, return code 1120
<libs>: Found error
<../../dist/bin/webapprt-stub.exe>: Found error
<../../dist/bin/webapprt-stub.exe>: Found error
<libs>: Found error
Executing: link -NOLOGO -OUT:webapprt-stub.exe -PDB:webapprt-stub.pdb -ENTRY:wmainCRTStartup -SUBSYSTEM:WINDOWS -LARGEADDRESSAWARE -NXCOMPAT -RELEASE -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -DEBUG -OPT:REF /HEAP:0x40000 @c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\webapprt\win\tmpkoxtjh.list module.res ..\..\dist\lib\xpcomglue_staticruntime.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib netapi32.lib shell32.lib
c:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\webapprt\win\tmpkoxtjh.list:
    webapprt.obj

xpcomglue_staticruntime.lib(nsCOMArray.obj) : error LNK2019: unresolved external symbol __imp__moz_xmalloc referenced in function "void * __cdecl operator new(unsigned int)" (??2@YAPAXI@Z)

xpcomglue_staticruntime.lib(nsCOMArray.obj) : error LNK2019: unresolved external symbol __imp__moz_free referenced in function "void __cdecl operator delete(void *)" (??3@YAXPAX@Z)

webapprt-stub.exe : fatal error LNK1120: 2 unresolved externals
(In reply to comment #5)
> Backed out

Try using nsMemory::Clone and nsMemory::Free perhaps?
After discussing with bsmedberg, I opted for NS_Alloc and NS_Free. This went green on try, so I repushed the changeset:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/2d31ac32628e
https://hg.mozilla.org/mozilla-central/rev/2d31ac32628e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.