Fix some of the failing COM unittests #578

matthiasblaesing · 2016-01-12T14:19:24Z

I had a look at the failing COM unittests and especially the failing CallbackHandlers.

For details please refer to the comments in the commits - I tried to explain the reasoning in the commit messages.

twall · 2016-01-12T19:55:08Z

contrib/platform/src/com/sun/jna/platform/win32/Guid.java

     */
-    public class REFIID extends IID {
+    public class REFIID extends ByReference {


This only makes a difference when the Structure is used as a field within a Structure. It's better to leave the main definition untagged and use an explicitly tagged inner class which derives from the main one.

Is REFIID simply an alias for IID*?

That was my first thought and would align with the FAQ about this, but then this does not explain the behavior in callbacks - (please see the comment for the first commit, first paragraph)

The difference between my explicit use of ByReference and the suggested Structure/Structure.ByReference is the special casing the latter receives in com.sun.jna.CallbackReference (lines 511-516). My approach circumvents the autowrite.

The alternative would have been to mark REFIID not to autosync, but that looked worse.

Yes REFIID is typedeffed to IIID* (https://msdn.microsoft.com/en-us/library/cc237816.aspx)

I think I see your point, but I fail to see how your strategy should be any different; it would seem to be a bug exploiting different code paths somehow, which should be resolved more systemically.

My reasoning for the com.sun.jna.ptr.ByReference approach was that it is closest to the underlying definition. It is clear by that no magic handling is expected from JNA, just passing the pointer.

If this is strictly out of line - I only see the Option to make the members of the GUID structure final so that when passed in they are only read and in general GUID must be interpreted as a value type.

My concern is that this has exposed a latent bug and we’d only be masking it.

Your definition is otherwise sane and certainly not misleading (except that it varies from the recommended method of establishing “struct” vs “struct *”).

Give me a few days to take a look at this (I’ve been totally swamped this week paying face time to visiting dignitaries).

T.

On Jan 13, 2016, at 11:03 AM, matthiasblaesing notifications@github.com wrote:

In contrib/platform/src/com/sun/jna/platform/win32/Guid.java:

*/

public class REFIID extends IID {

public class REFIID extends ByReference {

My reasoning for the com.sun.jna.ptr.ByReference approach was that it is closest to the underlying definition. It is clear by that no magic handling is expected from JNA, just passing the pointer.

If this is strictly out of line - I only see the Option to make the members of the GUID structure final so that when passed in they are only read and in general GUID must be interpreted as a value type.

—
Reply to this email directly or view it on GitHub.

ok, I misread this. You should implement REFIID as a PointerType (I though you were using Structure.ByReference rather than com.sun.jna.ptr.ByReference). com.sun.jna.ptr.ByReference should generally only be used when your called function expects a pointer to something else in order to "return" a value via parameter.

If you change REFIID to PointerType your code should still work and will not be ambiguous or misleading.

matthiasblaesing · 2016-01-25T14:46:54Z

Hello Timothy, did you find some time to look at this?

twall · 2016-01-25T16:57:22Z

Not yet, had some issues spinning up my windows VMs.

This one’s on my plate as well as the stdcall issue.

On Jan 25, 2016, at 9:46 AM, matthiasblaesing notifications@github.com wrote:

Hello Timothy, did you find some time to look at this?

—
Reply to this email directly or view it on GitHub.

twall · 2016-02-14T16:14:52Z

writeFieldsToMemory seems superfluous to Structure.write().

After the change introduced in d7f91f1 native callbacks for the COM handling error out (see below). Analysing the stack trace this happens after the java function was called. There structures are syncing back from java to native. I asume the calling programm passes in a readonly version of the IID and so the write fails. This code path was not hit before the above mentioned changeset, because there is a typeguard, that prevents syncing for ByValue calls (which were removed be the changeset). In addition to this, a fix ComEventCallbacks_Test to not depend on an installed office was commited. Now the Internet Explorer is used, as it can be expected to be present. Tests from ConnectionPointerContainer_Test duplicated a good part of ComEventCallbacks_Test so the cases were integrated into ComEventCallbacks_Test. The exception leading to this fix (CallbackReference.java:513 synchronised structed passed by reference after the java invocation): JNA: Callback com.sun.jna.platform.win32.COM.DispatchListener$1@233c0b17 threw the following exception: java.lang.Error: Invalid memory access at com.sun.jna.Native.setInt(Native Method) at com.sun.jna.Pointer.setInt(Pointer.java:1124) at com.sun.jna.Pointer.setValue(Pointer.java:925) at com.sun.jna.Structure.writeField(Structure.java:842) at com.sun.jna.Structure.write(Structure.java:754) at com.sun.jna.Structure.autoWrite(Structure.java:2047) at com.sun.jna.CallbackReference$DefaultCallbackProxy.invokeCallback(CallbackReference.java:513) at com.sun.jna.CallbackReference$DefaultCallbackProxy.callback(CallbackReference.java:528) at com.sun.jna.Native.invokeInt(Native Method) at com.sun.jna.Function.invoke(Function.java:390) at com.sun.jna.Function.invoke(Function.java:323) at com.sun.jna.Function.invoke(Function.java:275) at com.sun.jna.Function.invoke(Function.java:266) at com.sun.jna.platform.win32.COM.COMInvoker._invokeNativeObject(COMInvoker.java:37) at com.sun.jna.platform.win32.COM.ConnectionPoint.Advise(ConnectionPoint.java:42) at com.sun.jna.platform.win32.COM.ComEventCallbacks_Test.cause_Quit_Event(ComEventCallbacks_Test.java:236) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:497) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229) at org.junit.runners.ParentRunner.run(ParentRunner.java:309) at junit.framework.JUnit4TestAdapter.run(JUnit4TestAdapter.java:38) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:532) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:1179) at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:1030)

Depending on the context the unmarshalling of the supplied data in callbacks is unwrapped further than before. The NavigateComplete2 event from the interface DWebBrowserEvents2 demonstrates this. The URL is passed as a Variant pointing to a Variant containing the string. Without this fix NULL is returned. To test this ComEventCallbacks_Test.java was moved to use Internet Explorer instead of MS Office.

…ng thread Dispatching the invocation of the callback handler into an executor makes it impossible to fill [out] parameters, as the return has already happend and [in] parameters can not be savely used if they are not marshalled to java code because the calling code will free the parameters after the call. To prevent deadlocks ComThread is modified to allow COM calls from the callback by modifying the ComThread helper to only dispatch the COM call into the ComThread only if the calling thread has not COM already enabled. Reference counting was modified, so that now on construction of a ProxyObject the reference count is AddRef'ed once and Released once on finalization.

According to MSDN the caller is responsible to free BSTRs.

matthiasblaesing · 2016-02-21T16:13:05Z

Thank you for the feedback - I updated the pull request:

rebased onto master
based REFIID on PointerType instead of c.s.j.ptr.ByReference
fix native memory leak - this was discovered when investigating the calling in ProxyObject

The memory leak is based on the description in MSDN:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms686638%28v=vs.85%29.aspx

The caller is responsible for freeing memory in all cases. The most obvious case are the strings that are converted from/to BSTRs. The BSTRs are allocated as native memory when wrapped into a VARIANT, that memory is currently nowhere freed.

The freeing is only done for values that pass through the automatic conversion - if users directly use VARIANTs they are responsible to free it themselves.

I could not place your comment with regard to "writeFieldsToMemory" though - I assume this is directed to another pull?

twall · 2016-02-21T22:02:22Z

Take a look at the implementation of NativeString, which is used when a String is used as an argument and requires a native "copy" for the duration of a call.

twall · 2016-02-21T22:05:05Z

I think the comment about writeFieldsToMemory was referring to this usage.

twall · 2016-02-21T22:07:09Z

contrib/platform/test/com/sun/jna/platform/win32/COM/util/ComEventCallbacks_Test.java

-		@ComProperty
-		String getText();
+                /**
+                 * navOpenInNewWindow = 1


Would it make sense to explicitly expose these values as constants?

LGTM, nice work. The `writeFieldsToMemory`, `Convert` class, and any `NativeString` improvements can be addressed in a separate PR.

twall · 2016-02-21T22:17:46Z

LGTM, nice work. The writeFieldsToMemory, Convert class, and any NativeString improvements can be addressed in a separate PR.

dblock · 2016-02-22T12:08:28Z

👍

matthiasblaesing · 2016-02-22T19:04:53Z

Thanks for merging this! I'll revisit the mentioned cases and added them to my internal todo list. I have a few things (SAFEARRAY, threading) I'd like to get to first and I can't make a hard promise with regard to time I can invest.

Motivation: We recently had a bug which only showed up when the memoryAddress of a direct buffer could not be accessed directly. We didnt catch this as the memory address always was accessible by unsafe. Let's add a PR build job which explicit disable unsafe. Modifications: - Add profile which disables unsafe during build - Add job which builds with unsafe disabled Result: Better test coverage

twall reviewed Jan 12, 2016
View reviewed changes

matthiasblaesing added 5 commits February 21, 2016 16:35

Correct freeing of BSTRs

78e6d0f

According to MSDN the caller is responsible to free BSTRs.

Added CHANGES.md entry

ad12bf7

matthiasblaesing force-pushed the fix_com branch from 8d60f93 to ad12bf7 Compare February 21, 2016 15:44

twall reviewed Feb 21, 2016
View reviewed changes

twall added a commit that referenced this pull request Feb 21, 2016

Merge pull request #578 from matthiasblaesing/fix_com

bf32054

LGTM, nice work. The `writeFieldsToMemory`, `Convert` class, and any `NativeString` improvements can be addressed in a separate PR.

twall merged commit bf32054 into java-native-access:master Feb 21, 2016

matthiasblaesing deleted the fix_com branch February 22, 2016 19:00

matthiasblaesing mentioned this pull request Mar 7, 2016

ComEventCallbacks_Test: JVM crash #408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some of the failing COM unittests #578

Fix some of the failing COM unittests #578

matthiasblaesing commented Jan 12, 2016

twall Jan 12, 2016

matthiasblaesing Jan 12, 2016

twall Jan 13, 2016

matthiasblaesing Jan 13, 2016

twall Jan 15, 2016

twall Feb 14, 2016

matthiasblaesing commented Jan 25, 2016

twall commented Jan 25, 2016

twall commented Feb 14, 2016

matthiasblaesing commented Feb 21, 2016

twall commented Feb 21, 2016

twall commented Feb 21, 2016

twall Feb 21, 2016

twall commented Feb 21, 2016

dblock commented Feb 22, 2016

matthiasblaesing commented Feb 22, 2016

Fix some of the failing COM unittests #578

Fix some of the failing COM unittests #578

Conversation

matthiasblaesing commented Jan 12, 2016

twall Jan 12, 2016

Choose a reason for hiding this comment

matthiasblaesing Jan 12, 2016

Choose a reason for hiding this comment

twall Jan 13, 2016

Choose a reason for hiding this comment

matthiasblaesing Jan 13, 2016

Choose a reason for hiding this comment

twall Jan 15, 2016

Choose a reason for hiding this comment

twall Feb 14, 2016

Choose a reason for hiding this comment

matthiasblaesing commented Jan 25, 2016

twall commented Jan 25, 2016

twall commented Feb 14, 2016

matthiasblaesing commented Feb 21, 2016

twall commented Feb 21, 2016

twall commented Feb 21, 2016

twall Feb 21, 2016

Choose a reason for hiding this comment

twall commented Feb 21, 2016

dblock commented Feb 22, 2016

matthiasblaesing commented Feb 22, 2016