InstanceContent.remove(obj) fails if obj has mutated and lookup contains 12+ items

[jjazzboss]

Apache NetBeans version

Apache NetBeans 29

What happened

This is probably an old bug (present in NB7). I experienced it some years ago and could not reproduce. I avoided it by using immutable objects in my lookup and creating a custom identity-based AbstractLookup.Content.

Today I asked copilot to investigate and it seems to have found the issue. I did not check NB code myself but I did check the generated unit tests that reproduce the issue.

Language / Project Type / NetBeans Component

Lookup API

How to reproduce

/*
 * Standalone JUnit 5 test demonstrating the NetBeans AbstractLookup mutation bug (NB28 but way older than that).
 *
 * Bug summary:
 *   AbstractLookup switches from ArrayStorage (linear scan) to InheritanceTree (hash-based
 *   LinkedHashSet) once the total number of registered pairs exceeds 11 (the default threshold).
 *   If a stored object mutates and its hashCode() changes while in the InheritanceTree,
 *   InstanceContent.remove() silently fails because it looks in the wrong hash bucket.
 *
 * Fix: use IdentityInstanceContent which relies on System.identityHashCode() and == instead of hashCode()/equals(), 
 * making remove() immune to any mutation of the stored objects.
 */
package org.jjazz.chordleadsheet;

import java.util.Collection;
import java.util.concurrent.Executor;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.*;
import org.openide.util.lookup.AbstractLookup;
import org.openide.util.lookup.InstanceContent;

/**
 * Tests that expose a silent remove() failure in NetBeans AbstractLookup when a stored mutable object's hashCode() changes after being added to the lookup.
 */
public class LookupMutationTest
{

    // ---------------------------------------------------------------------------
    // Helper: a mutable object with field-based hashCode(). 
    // ---------------------------------------------------------------------------
    static class Mutable
    {

        int value;

        Mutable(int v)
        {
            this.value = v;
        }

        /**
         * Purely field-based — no identity shortcut.
         */
        @Override
        public boolean equals(Object o)
        {
            if (!(o instanceof Mutable other))
            {
                return false;
            }
            return this.value == other.value;
        }

        @Override
        public int hashCode()
        {
            return value;
        }

        @Override
        public String toString()
        {
            return "Mutable(" + value + ")";
        }
    }

    // ---------------------------------------------------------------------------
    // Helper: dummy type used as filler to push the total pair count above the
    // ArrayStorage threshold (11), forcing AbstractLookup to switch to InheritanceTree.
    // ---------------------------------------------------------------------------
    static class Filler
    {
    }

    // ---------------------------------------------------------------------------
    // Test 1 — lookup() always works after mutation.
    //
    // lookup() searches by type using Pair.instanceOf(), which calls
    // Class.isInstance(obj). This is independent of hashCode(), so mutation never
    // breaks it.
    // ---------------------------------------------------------------------------
    @Test
    public void testLookupAlwaysWorksAfterMutation()
    {
        var obj = new Mutable(42);
        var ic = new InstanceContent();
        var lookup = new AbstractLookup(ic);
        ic.add(obj);

        assertSame(obj, lookup.lookup(Mutable.class), "lookup() should return obj before mutation");

        int hashBefore = obj.hashCode();
        obj.value = 99;
        assertNotEquals(hashBefore, obj.hashCode(), "hashCode() should have changed after mutation");

        // lookup() is unaffected — it finds the object by type, not by hash
        assertSame(obj, lookup.lookup(Mutable.class), "lookup() should still return obj after mutation");
    }

    // ---------------------------------------------------------------------------
    // Test 2 — remove() is safe with ArrayStorage (≤ 11 items total).
    //
    // With ≤ 11 total pairs, AbstractLookup uses ArrayStorage, which removes items
    // by iterating a plain Object[] and calling arr[i].equals(removalPair).
    // Because both the stored pair and the removal pair wrap the same object
    // reference, equals() compares the object's current value to itself — always
    // true — so remove() succeeds regardless of hash changes.
    // ---------------------------------------------------------------------------
    @Test
    public void testRemoveWorksWithArrayStorage()
    {
        var obj1 = new Mutable(10);
        var obj2 = new Mutable(20);
        var obj3 = new Mutable(30);

        var ic = new InstanceContent();
        var lookup = new AbstractLookup(ic);

        // 3 items — well within the 11-item ArrayStorage threshold
        ic.add(obj1);
        ic.add(obj2);
        ic.add(obj3);

        obj2.value = 99; // mutate: hashCode changes

        ic.remove(obj2); // ArrayStorage: linear scan, equals() comparison → succeeds

        Collection<? extends Mutable> result = lookup.lookupAll(Mutable.class);
        assertFalse(result.contains(obj2), "obj2 should have been removed (ArrayStorage path)");
        assertEquals(2, result.size(), "Only obj1 and obj3 should remain");
    }

    // ---------------------------------------------------------------------------
    // Test 3 — remove() silently FAILS with InheritanceTree storage (> 11 items).
    //          THIS IS THE BUG.
    //
    // When the total pair count exceeds 11, AbstractLookup upgrades to
    // InheritanceTree, which stores pairs in a LinkedHashSet<Pair> (backed by a
    // LinkedHashMap). Java's HashMap caches the hash of each key at insertion time
    // in Node.hash. After mutation, the removal pair's hashCode() returns a new
    // value, so HashMap looks in the wrong bucket and silently returns false.
    // The object stays stuck in the lookup.
    // ---------------------------------------------------------------------------
    @Test
    public void testRemoveFailsWithInheritanceTreeStorage()
    {
        var obj1 = new Mutable(10);
        var obj2 = new Mutable(20);
        var obj3 = new Mutable(30);

        var ic = new InstanceContent();
        var lookup = new AbstractLookup(ic);

        // 9 fillers + 3 Mutables = 12 total → exceeds threshold → InheritanceTree
        for (int i = 0; i < 9; i++)
        {
            ic.add(new Filler());
        }
        ic.add(obj1);
        ic.add(obj2);
        ic.add(obj3);

        // Trigger type-node creation in InheritanceTree (moves pairs to a typed LinkedHashSet node)
        lookup.lookupAll(Mutable.class);

        obj2.value = 99; // hashCode: hash(20) → hash(99) — stored node is still in bucket hash(20)!

        ic.remove(obj2); // InheritanceTree: looks in bucket hash(99) → miss → returns false silently

        Collection<? extends Mutable> result = lookup.lookupAll(Mutable.class);
        assertTrue(result.contains(obj2),
                "BUG: obj2 is stuck in the lookup — InstanceContent.remove() failed silently after mutation "
                + "because InheritanceTree uses a hash-based LinkedHashSet whose cached bucket hash is now stale.");
    }

    // ---------------------------------------------------------------------------
    // Test 4 — IdentityInstanceContent fixes the bug.
    //
    // By using System.identityHashCode() and == instead of hashCode()/equals(),
    // the hash bucket computed for add() and remove() is always the same (the JVM
    // identity hash is stable for the lifetime of the object), regardless of any
    // field mutation.
    // ---------------------------------------------------------------------------
    @Test
    public void testIdentityInstanceContentFixesBug()
    {
        var obj1 = new Mutable(10);
        var obj2 = new Mutable(20);
        var obj3 = new Mutable(30);

        var iic = new IdentityInstanceContent();
        var lookup = new AbstractLookup(iic);

        for (int i = 0; i < 9; i++)
        {
            iic.add(new Filler()); // push past threshold
        }
        iic.add(obj1);
        iic.add(obj2);
        iic.add(obj3);

        lookup.lookupAll(Mutable.class);

        obj2.value = 99; // same mutation as test 3

        iic.remove(obj2); // identity hash is stable → correct bucket → found and removed

        Collection<? extends Mutable> result = lookup.lookupAll(Mutable.class);
        assertFalse(result.contains(obj2),
                "IdentityInstanceContent.remove() should succeed even after mutation");
        assertEquals(2, result.size(), "Only obj1 and obj3 should remain");
    }

    // ===========================================================================
    // IdentityInstanceContent
    //
    // A drop-in replacement for InstanceContent that uses object identity
    // (System.identityHashCode / ==) instead of the object's own hashCode/equals.
    // This makes add/remove immune to mutations that change the object's hash.
    //
    // Equivalent to JJazzLab's MutableInstanceContent.
    // ===========================================================================

    static final class IdentityInstanceContent extends AbstractLookup.Content
    {

        public IdentityInstanceContent()
        {
        }

        public IdentityInstanceContent(Executor notifyIn)
        {
            super(notifyIn);
        }

        /**
         * Add an object to the lookup.
         */
        public final void add(Object inst)
        {
            addPair(new IdentityPair<>(inst));
        }

        /**
         * Remove an object from the lookup.
         */
        public final void remove(Object inst)
        {
            removePair(new IdentityPair<>(inst));
        }
    }

    /**
     * A Lookup Pair that uses object identity for equals() and hashCode().
     */
    private static final class IdentityPair<T> extends AbstractLookup.Pair<T>
    {

        private final T obj;

        IdentityPair(T obj)
        {
            if (obj == null)
            {
                throw new NullPointerException();
            }
            this.obj = obj;
        }

        @Override public boolean instanceOf(Class<?> c)
        {
            return c.isInstance(obj);
        }

        @Override public T getInstance()
        {
            return obj;
        }

        /**
         * Identity equality — immune to any mutation of obj.
         */
        @Override
        public boolean equals(Object o)
        {
            return (o instanceof IdentityPair<?> other) && obj == other.obj;
        }

        /**
         * Identity hash — stable for the lifetime of obj regardless of mutation.
         */
        @Override
        public int hashCode()
        {
            return System.identityHashCode(obj);
        }

        @Override public String getId()
        {
            return "Identity[" + obj + "]";
        }

        @Override public String getDisplayName()
        {
            return obj.toString();
        }

        @Override protected boolean creatorOf(Object o)
        {
            return obj == o;
        }

        @SuppressWarnings("unchecked")
        @Override public Class<? extends T> getType()
        {
            return (Class<? extends T>) obj.getClass();
        }
    }
}

Did this work correctly in an earlier version?

No / Don't know

Operating System

Win 11

JDK

Adoptium 23

Apache NetBeans packaging

Apache NetBeans binary zip

Anything else

No response

Are you willing to submit a pull request?

No

[mbien]

Wondering why you expect it to work if the instance identity changes? Does the doc say something about InstanceContent having to compare references (==) and ignore user provided equals() hashCode() etc?

Regular collections like HashSet won't work either if hashCode() of an object changes after it was added (you would have to use the Collections.newSetFromMap(new IdentityHashMap<>())).

[matthiasblaesing]

The implementation of InstanceContent has a single relevant statement (https://bits.netbeans.org/27/javadoc/org-openide-util-lookup/org/openide/util/lookup/InstanceContent.html):

If inst already exists in the lookup (equality is determined by object's Object.equals(java.lang.Object)) method) then the new instance replaces the old one in the lookup but listener notifications are not delivered in such case.

It does not say anything whether "identity" may change inside the lifecycle.

Simplest solution: Document the constraint (hashCode must not change) for InstanceContent.

[jjazzboss]

Wondering why you expect it to work if the instance identity changes?

Because it's not written anywhere. By comparison the javadoc of hash-based collections contain clear warnings, e.g. for Map: "Note: great care must be exercised if mutable objects are used as map keys".

And it's not obvious because :

• Lookup.lookup() will continue to work even if an object inside the lookup mutates
• as @matthiasblaesing said, InstanceContent.add() javadoc can be misleading
• InstanceContent.remove() DOES work with mutable objects, but only if there is less than 11 objects in the lookup

Simplest solution: Document the constraint (hashCode must not change) for InstanceContent.

Yes.

[mbien]

Wondering why you expect it to work if the instance identity changes?

Because it's not written anywhere.

We certainly can add a note to the javadoc, but please be aware that Maps and Sets are used everywhere (not just in NB).

Mutable hash code is a red flag and will fail in many situations. By default, virtually everything will expect it to be constant. (e.g I remember https://issues.apache.org/jira/browse/LUCENE-10431 and how tricky it was narrow down)

[neilcsmith-net]

A fairly good AI summary for once! 😄

If we add documentation, I would probably document in Lookup that mutable objects with changing hash code or equals should not be included in them? I think we have multiple other places that could be affected by similar issues.

[matthiasblaesing]

If we add documentation, I would probably document in Lookup that mutable objects with changing hash code or equals should not be included in them? I think we have multiple other places that could be affected by similar issues.

I would not change Lookup. That interface only covers the "read" side. And that is not affected by this. The crucial part is the lookup creator needs to define whether it will store identity-shifting objects or not. So I think AbstractLookup is the right place as that is the class that a caller needs to instantiate differently and has direct control over.

[neilcsmith-net]

I thought that at first, but there's code in Lookup and other subclasses that aren't necessarily related to AbstractLookup that might have issues with equals or hashcode changing after things have been added as well. So, in parallel to the example from Map above, I think it belongs at the higher level. Still, any warning, anywhere, or none is also fine IMO - not going to get into a big discussion on it if the consensus is elsewhere.