Repository: mockito/mockito
Issue: "The type is not public and its mock class is loaded by a different class loader" with a context classloader that delegates

Repro case:
```java
package com.google.clm.mockitobug;

import static org.mockito.Mockito.mock;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/**
 * My goal is to create a context classloader that is identical in every way to the existing one,
 * except that it has a different object identity and possibly some extra fields. However, even a
 * basic classloader that always delegates causes problems for mockito.
 */
@RunWith(JUnit4.class)
public final class ClassLoaderTest {

  @Test
  public void mockPackagePrivateInterface() {
    ClassLoader old = Thread.currentThread().getContextClassLoader();
    Thread.currentThread().setContextClassLoader(new ClassLoader(old) {});
    // fails
    Object unused = mock(PackagePrivate.class);
  }

  @Test
  public void mockPublicInterface() {
    ClassLoader old = Thread.currentThread().getContextClassLoader();
    Thread.currentThread().setContextClassLoader(new ClassLoader(old) {});
    // succeeds
    Object unused = mock(Public.class);
  }

  interface PackagePrivate {}

  public interface Public {}
}
```
Error:
```
org.mockito.exceptions.base.MockitoException: 
Mockito cannot mock this class: interface com.google.clm.mockitobug.ClassLoaderTest$PackagePrivate.

Mockito can only mock non-private & non-final classes.
If you're not sure why you're getting this error, please report to the mailing list.


Java               : 11
JVM vendor name    : Google Inc.
JVM vendor version : 11.0.10+9-google-release-371350251
JVM name           : OpenJDK 64-Bit Server VM
JVM version        : 11.0.10+9-google-release-371350251
JVM info           : mixed mode, sharing
OS name            : Linux
OS version         : 4.15.0-smp-912.23.0.0


Underlying exception : org.mockito.exceptions.base.MockitoException: 
Cannot create mock for interface com.google.clm.mockitobug.ClassLoaderTest$PackagePrivate

The type is not public and its mock class is loaded by a different class loader.
This can have multiple reasons:
 - You are mocking a class with additional interfaces of another class loader
 - Mockito is loaded by a different class loader than the mocked type (e.g. with OSGi)
 - The thread's context class loader is different than the mock's class loader
	at com.google.clm.mockitobug.ClassLoaderTest.mockPackagePrivateInterface(ClassLoaderTest.java:21)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:57)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:59)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:81)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:327)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:84)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:292)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:73)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:290)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:60)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:270)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:370)
	at com.google.testing.junit.runner.internal.junit4.CancellableRequestFactory$CancellableRunner.run(CancellableRequestFactory.java:108)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
	at com.google.testing.junit.runner.junit4.JUnit4Runner.run(JUnit4Runner.java:104)
	at com.google.testing.junit.runner.RunnerShell$2.run(RunnerShell.java:34)
	at com.google.testing.junit.runner.GoogleTestRunner.runTestsInSuite(GoogleTestRunner.java:200)
	at com.google.testing.junit.runner.GoogleTestRunner.runTestsInSuite(GoogleTestRunner.java:184)
	at com.google.testing.junit.runner.GoogleTestRunner.main(GoogleTestRunner.java:137)
Caused by: org.mockito.exceptions.base.MockitoException: 
Cannot create mock for interface com.google.clm.mockitobug.ClassLoaderTest$PackagePrivate

The type is not public and its mock class is loaded by a different class loader.
This can have multiple reasons:
 - You are mocking a class with additional interfaces of another class loader
 - Mockito is loaded by a different class loader than the mocked type (e.g. with OSGi)
 - The thread's context class loader is different than the mock's class loader
	at net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:153)
	at net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:366)
	at net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:175)
	at net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:377)
	... 27 more

```

If mockito actually tried to use the classloader, I think it would work. Since we don't see an interesting exception deep in the stack, my guess is that mockito is doing some extra validation to avoid generating bytecode it thinks would fail.

Issue comments:
Rafael, I took a look at this code and it wasn't completely clear to my why we are failing here. The reason that is probably happening is that Mockito first adds the mocked type (https://github.com/mockito/mockito/blob/6f9108b833323345bd926f10914224d3e0a11b80/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java#L97) in this case `PackagePrivate` before adding the current classloader: https://github.com/mockito/mockito/blob/6f9108b833323345bd926f10914224d3e0a11b80/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java#L99

So it looks up the classloader for the mocked type, which is the old classloader. Then it looks up the current classloader (which you changed) and then determines that they don't match. Thus `localMock == false`.

This code landed as part of supporting Java 9+ modules: https://github.com/mockito/mockito/pull/1582 It wouldn't surprise me if there is an edge case we are missing.

Since the new classloader delegates to the former, I would have expected the classes to match and thus pass the checks, but they don't. Presumably because of the classloader equality check. Can we be lenient here and allow for child classloaders to be valid as well?
The reason we fail is that a package-private type is not visible outside of its own class loader. Even if we defined the subclass in the same nominal package, if it was loaded by a different class loader, it could not see its superclass. To avoid this visibility issue, we fail before even attempting to create the mock.

We could consider to drop the context class loader if a mocked class is package-private. I am not sure why we need the context class loader to begin with but I assume its related to serialization what tends to be an edge case.
I can think of a few options:

1. Ignore the context classloader if the type is package-private. It could have failed anyway... so if it succeeds, might as well reap the benefit.
2. Ignore the package-private restriction for classloaders that are not participating in the java module system, by checking isSealed on the class's package
3. Check to see if the context classloader is a child of the originating classloader, although if a classloader is declaring a parent but not following the delegation patter this might cause a false negative or positive.
4. Just load the stuff as you would, and check each relevant class you load and error if the defining classloader doesn't match.
I'll mess around a bit more and report back if I figure out what the issue is.
OK, I have a the minimal fix (as in, it will only affect cases that would have broken before):

If the mocked type is a non-interface or non-public, or any of the extra interfaces are non-public, don't include the context classloader in MultipleParentsClassLoader iff the context classloader is a child of the classloader we'd use if it wasn't included.

This fixes my issue, and in general likely fixes cases where a context classloader is set that follows the parent-first delegation pattern. I thought about simply removing the context loader entirely, but test authors who hit this can set and unset it in a try-finally block around their mock calls if necessary, and since there's no comments about why it's included I'd be wary of breaking somebody depending on the existing behavior.

I am going to run this fix through google's internal suite of tests to see if it breaks anything. 
1. The problem is that it might fail delayed upon deserialization. This might be non-trivial to understand and we should document this restriction somewhere. 
2. That won't work. Sealing only affects jars on the same class loader that are defining the same package, this won't work with modules anymore anyways and is taken care of. Package-private classes are however never visible on other class loaders, therefore we must retain the class loader.
3. Is already done to some extend.
4. This is implicit by the multiple parent loader.

I think the right approach is to exclude the context loader for package-private classes by default since it will never work and to fail if serialization is enabled in addition. 
> I think the right approach is to exclude the context loader for package-private classes by default since it will never work and to fail if serialization is enabled in addition.

It's not just package-private classes - mocking a public non-final-non-interface class also poses problems, since that will break stubbing/verification of package-private methods (#796).

My proposed fix:
```java
private static boolean needsSamePackageClassLoader(MockFeatures<?> features) {
  if (!Modifier.isPublic(features.mockedType.getModifiers())
                              || !features.mockedType.isInterface()) {
    // The mocked type is package private or is not an interface and thus may contain package
    // private methods.
    return true;
  }
  for (Class<?> iface : features.interfaces) {
    if (!Modifier.isPublic(iface.getModifiers())) {
      return true;
    }
  }
  return false;
}
...
MultipleParentClassLoader.Builder loaderBuilder = new MultipleParentClassLoader.Builder()
                  .appendMostSpecific(getAllTypes(features.mockedType))
                  .appendMostSpecific(features.interfaces)
                  .appendMostSpecific(MockAccess.class);
ClassLoader contextLoader = currentThread().getContextClassLoader();
boolean shouldIncludeContextLoader = true;
if (needsSamePackageClassLoader(features)) {
    // For the generated class to access package-private methods, it must be defined by the
    // same classloader as its type. All the other added classloaders are required to load
    // the type; if the context classloader is a child of the mocked type's defining
    // classloader, it will break a mock that would have worked. Check if the context class
    // loader is a child of the classloader we'd otherwise use, and possibly skip it.
    ClassLoader candidateLoader = loaderBuilder.build();
    for (ClassLoader parent = contextLoader; parent != null; parent = parent.getParent()) {
      if (parent == candidateLoader) {
        shouldIncludeContextLoader = false;
        break;
      }
    }
}
if (shouldIncludeContextLoader) {
  loaderBuilder = loaderBuilder.appendMostSpecific(contextLoader);
}
ClassLoader classLoader = loaderBuilder.build();
```

The focus was on only changing behavior for cases that failed before. Since we verify that the classloader we define in is a parent of the context classloader, we shouldn't see any problems with serialization, right? Or at least no new problems, as the existing `MultipleParentsClassLoader` relies on `getParent()` to infer the delegation order.
@charlesmunger Do you mind opening a PR that includes your proposed fix and adds a regression test for your use case? If all tests pass, I am inclined to merge as-is. If we end up breaking anybody else, we should add regression tests for their use cases and modify the implementation accordingly.

Pull request review comments:
I'd prefer to avoid the static import here as it's the only use and we tend to not use static imports.
Not quite sure, why not most specific anymore?
Move comment to one line.
Interfaces might declare interfaces of their own.
We already did this on line 99 in the current implementation, so we don't change this here. Can we clean these up in a follow-up?
That's OK, as all interface methods are public, and a public interface that extends a package-private interface doesn't require the implementation to be in the same package. All that matters for the generated type is that the directly implemented interfaces are accessible.
Fixed in new commit, I originally based off a different branch I think
Let's figure this out in a follow-up. I think we need to reconfigure the formatter to do this.
Of course, my bad. I was thinking that you iterated over the interfaces of the class but it's of course the directly implemented ones that matter.
Agreed.

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java
index efe977fa85..edb9640afb 100644
--- a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java
+++ b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java
@@ -90,15 +90,53 @@ protected SubclassBytecodeGenerator(
         handler = ModuleHandler.make(byteBuddy, loader, random);
     }
 
+    private static boolean needsSamePackageClassLoader(MockFeatures<?> features) {
+        if (!Modifier.isPublic(features.mockedType.getModifiers())
+                || !features.mockedType.isInterface()) {
+            // The mocked type is package private or is not an interface and thus may contain
+            // package private methods.
+            return true;
+        }
+        for (Class<?> iface : features.interfaces) {
+            if (!Modifier.isPublic(iface.getModifiers())) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     @Override
     public <T> Class<? extends T> mockClass(MockFeatures<T> features) {
-        ClassLoader classLoader =
+        MultipleParentClassLoader.Builder loaderBuilder =
                 new MultipleParentClassLoader.Builder()
-                        .appendMostSpecific(getAllTypes(features.mockedType))
+                        .appendMostSpecific(features.mockedType)
                         .appendMostSpecific(features.interfaces)
-                        .appendMostSpecific(currentThread().getContextClassLoader())
-                        .appendMostSpecific(MockAccess.class)
-                        .build();
+                        .appendMostSpecific(
+                                MockAccess.class, DispatcherDefaultingToRealMethod.class)
+                        .appendMostSpecific(
+                                MockMethodInterceptor.class,
+                                MockMethodInterceptor.ForHashCode.class,
+                                MockMethodInterceptor.ForEquals.class);
+        ClassLoader contextLoader = currentThread().getContextClassLoader();
+        boolean shouldIncludeContextLoader = true;
+        if (needsSamePackageClassLoader(features)) {
+            // For the generated class to access package-private methods, it must be defined by the
+            // same classloader as its type. All the other added classloaders are required to load
+            // the type; if the context classloader is a child of the mocked type's defining
+            // classloader, it will break a mock that would have worked. Check if the context class
+            // loader is a child of the classloader we'd otherwise use, and possibly skip it.
+            ClassLoader candidateLoader = loaderBuilder.build();
+            for (ClassLoader parent = contextLoader; parent != null; parent = parent.getParent()) {
+                if (parent == candidateLoader) {
+                    shouldIncludeContextLoader = false;
+                    break;
+                }
+            }
+        }
+        if (shouldIncludeContextLoader) {
+            loaderBuilder = loaderBuilder.appendMostSpecific(contextLoader);
+        }
+        ClassLoader classLoader = loaderBuilder.build(MockMethodInterceptor.class.getClassLoader());
 
         // If Mockito does not need to create a new class loader and if a mock is not based on a JDK
         // type, we attempt


Test patch:
diff --git a/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java b/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java
new file mode 100644
index 0000000000..e01365ba99
--- /dev/null
+++ b/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java
@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2017 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockitousage.bugs.creation;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Regression tests for issue #2303*/
+public class PackagePrivateWithContextClassLoaderTest {
+
+    private ClassLoader oldContextClassloader;
+
+    public static class PublicClass {
+
+        int packagePrivateMethod() {
+            return 0;
+        }
+    }
+
+    public interface PublicInterface {}
+
+    interface PackagePrivateInterface {}
+
+    static class PackagePrivateClass {}
+
+    @Before
+    public void setUp() {
+        oldContextClassloader = Thread.currentThread().getContextClassLoader();
+        Thread.currentThread().setContextClassLoader(new ClassLoader(oldContextClassloader) {});
+    }
+
+    @After
+    public void teardown() {
+        Thread.currentThread().setContextClassLoader(oldContextClassloader);
+    }
+
+    @Test
+    public void should_be_able_to_mock_package_private_method() throws Exception {
+        PublicClass publicClass = mock(PublicClass.class);
+        when(publicClass.packagePrivateMethod()).thenReturn(3);
+        assertThat(publicClass.packagePrivateMethod()).isEqualTo(3);
+    }
+
+    @Test
+    public void should_be_able_to_mock_package_private_class() throws Exception {
+        PackagePrivateClass mock = mock(PackagePrivateClass.class);
+    }
+
+    @Test
+    public void should_be_able_to_mock_package_private_interface() throws Exception {
+        PackagePrivateInterface mock = mock(PackagePrivateInterface.class);
+    }
+
+    @Test
+    public void should_be_able_to_mock_package_private_extra_interface() throws Exception {
+        PackagePrivateInterface mock =
+                (PackagePrivateInterface)
+                        mock(
+                                PublicInterface.class,
+                                withSettings().extraInterfaces(PackagePrivateInterface.class));
+    }
+}