Add disposing check

[omares]

According to the disposing pattern one should check the disposing flag.

This also fixes the JNI DETECTED ERROR IN APPLICATION: jarray was NULL error

[jamesathey]

According to the disposing pattern, one should always dispose unmanaged resources, because the finalizer will not do it for you.

That said, in FastJavaByteArray.cs, it should probably look like this:

    protected override void Dispose(bool disposing)
    {
        unsafe
        {
            if (Raw != null)
                // tell Java that we're done with this array
                JniEnvEx.ReleaseByteArrayElements(Handle, Raw, IsReadOnly ? PrimitiveArrayReleaseMode.Release : PrimitiveArrayReleaseMode.CommitAndRelease);
            Raw = null;
        }
        base.Dispose(disposing);
    }

[omares]

Hmm, i am not an dispose expert so i researched a bit and found following information.

1. The finalize method (which basically is the destructor) gets called when the GC tries to get rid of an object because its not used anymore. That basically should not happen as in a perfect world one calls Dispose when the object is not needed anymore.
2. Its recommended to check the dispose parameter to differentiate between a dispose (true) and finalize (false) call.
3. The finalizer/destructor should simply call Dispose(false)
4. A call to GC.SuppressFinalize is currently missing to suppress later calls from the finalizer, which probably leads to the current issue
5. When implementing the SuppressFinalize call, a Raw != null check should not be needed anymore as the GC would not touch the object anymore. This requires testing as i am not sure about that.

How about this solution? This follows the dispose/finalizer pattern as good as possible. I kept the raw check in it, just to be safe ;)

protected virtual void Dispose(bool disposing)
{
    unsafe
    {
        // we always want to do that no matter if dispose or finalize call
        // the if check is probably not needed
        if (Raw != null) {
            JniEnvEx.ReleaseByteArrayElements(Handle, Raw, IsReadOnly ? PrimitiveArrayReleaseMode.Release : PrimitiveArrayReleaseMode.CommitAndRelease);
            Raw = null;
        }

        // we disposed by hand, surpress finalizer
        if (disposing) {
            GC.SuppressFinalize(this);
        }

    }

    base.Dispose(disposing);
}

~ FastJavaByteArray()
{
    Dispose(false);
}

The SO posts contains valuable knowledge :)

[jamesathey]

FastJavaByteArray inherits from Java.Lang.Object. Its Dispose(bool) is an override of the base class version. The base class already implements public void Dispose(), non-virtual, and that's where GC.SuppressFinalize(this) gets called (as recommended by the IDisposable pattern). According to the disassembly, the parent class Dispose methods look like this:

public void Dispose()
{
    this.Dispose(true);
    Object.Dispose(this, ref this.handle, this.key_handle, this.handle_type);
    GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing)
{
}

internal static void Dispose(object instance, ref IntPtr handle, IntPtr key_handle, JObjectRefType handle_type)
{
    if (handle == IntPtr.Zero)
    {
        return;
    }
    if (Logger.LogGlobalRef)
    {
        Logger.Log(LogLevel.Info, "monodroid-gref", string.Format("Disposing handle 0x{0}", handle.ToString("x")));
    }
    Object.DeregisterInstance(instance, key_handle);
    if (handle_type != JObjectRefType.Global)
    {
        if (handle_type != JObjectRefType.WeakGlobal)
        {
            throw new InvalidOperationException("Trying to dispose handle of type '" + handle_type + "' which is not supported.");
        }
        lock (instance)
        {
            JNIEnv.DeleteWeakGlobalRef(handle);
            handle = IntPtr.Zero;
        }
    }
    else
    {
        lock (instance)
        {
            JNIEnv.DeleteGlobalRef(handle);
            handle = IntPtr.Zero;
        }
    }
}

[jamesathey]

One other thing - Java.Lang.Object has a finalizer, which looks like this:

~Object()
{
    if (Logger.LogGlobalRef)
    {
        Logger.Log(LogLevel.Info, "monodroid-gref", string.Format("Finalizing handle 0x{0}", this.handle.ToString("x")));
    }
    this.refs_added = 0;
    if (this.handle != IntPtr.Zero)
    {
        GC.ReRegisterForFinalize(this);
    }
    else
    {
        this.Dispose(false);
    }
}

According to "Implementing Finalize and Dispose to Clean Up Unmanaged Resources" on MSDN,

The derived class does not have a Finalize method or a Dispose method without parameters because it inherits them from the base class.

That's why FastJavaByteArray only has the one Dispose(bool) method overriding the base class version.

[omares]

Okay i see! Thank you for your patience and clarification.

As the changes are already applied in your repo, ill close my PR.