Comment 1 Olivier Thomann 2006-05-04 12:46:18 EDT

I will investigate.

Comment 2 Olivier Thomann 2006-05-04 13:16:56 EDT

Closing as WORKSFORME. Since it is not possible to reproduce it and looking at the source code, we don't see how this is possible.

Comment 3 Olivier Thomann 2006-11-28 12:43:38 EST

*** Bug 166076 has been marked as a duplicate of this bug. ***

Comment 4 Olivier Thomann 2006-11-28 13:01:28 EST

Reopening since we need to understand how this can happened.

Comment 5 Markus Keller 2006-11-28 13:26:46 EST

(In reply to bug 166076 comment #3)
Must be a concurrency issue then. The method is not synchronized and can be called from multiple threads (e.g. compiler and type hierarchy resolver). It is possible that one thread sets this.sourceName temporarily to null just before the arraycopy takes place.

Comment 6 Olivier Thomann 2006-11-28 13:58:19 EST

The creation of binding should be synchronized. I don't see why the class file reader would need to be synchronized.
Kent, is this doable?

Comment 7 Kent Johnson 2006-11-28 16:03:35 EST

How is the instance of the ClassFileReader available to multiple threads?

BinaryTypeBindings create & initialize themselves from the ClassFileReader then throw it away.

Is this the source we're talking about on line 567?

  this.sourceName = new char[name.length - start];
>>System.arraycopy(name, start, this.sourceName, 0, this.sourceName.length);

Looks like a bogus VM/jit bug to me.

What VM are you guys using?

Comment 8 Benno Baumgartner 2006-11-29 04:33:31 EST

(In reply to comment #7)
> How is the instance of the ClassFileReader available to multiple threads?
> 
> BinaryTypeBindings create & initialize themselves from the ClassFileReader then
> throw it away.

Are you sure about that? I find it hard to verify that there is no aliasing. i.e. HierarchyResolver.remember stores the object in an array. The object is retrieved in LookupEnvironment.askForType(PackageBinding, char[]) from NameEvironmentAnswer#getBinaryType() which looks shared to me.

> 
> Is this the source we're talking about on line 567?
> 
>   this.sourceName = new char[name.length - start];
> >>System.arraycopy(name, start, this.sourceName, 0, this.sourceName.length);
> 
> Looks like a bogus VM/jit bug to me.
> 
> What VM are you guys using?

java version "1.5.0_08"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_08-b03)
Java HotSpot(TM) Client VM (build 1.5.0_08-b03, mixed mode)

Comment 9 Markus Keller 2006-11-29 18:33:17 EST

(In reply to comment #7)
> How is the instance of the ClassFileReader available to multiple threads?
Easy to see in the debugger:
- debug a cleaned runtime workspace
- create an empty Java project
- open editor for Integer
- set a breakpoint in ClassFileReader on line 554 and restart the runtime
=> debugger hangs in two threads
- resume the thread that is building a type hierarchy
=> now, you have two threads working on the same ClassFileReader, and the NPE can be provoked by "stepwise refinement"

> Is this the source we're talking about on line 567?
Yes

Comment 10 Kent Johnson 2006-11-30 15:39:08 EST

Here are the stack traces from Markus' steps :

The LookupEnvironment & BinaryTypeBinding instances are not the same, but the ClassFileReader instances are.

Thread [Worker-1] (Suspended (breakpoint at line 554 in ClassFileReader))
ClassFileReader.getSourceName() line: 554
BinaryTypeBinding.<init>(PackageBinding, IBinaryType, LookupEnvironment) line: 150
LookupEnvironment.createBinaryTypeFrom(IBinaryType, PackageBinding, boolean, AccessRestriction) line: 560
LookupEnvironment.createBinaryTypeFrom(IBinaryType, PackageBinding, AccessRestriction) line: 557
CompilationUnitResolver(Compiler).accept(IBinaryType, PackageBinding, AccessRestriction) line: 272
LookupEnvironment.askForType(PackageBinding, char[]) line: 127
PackageBinding.getTypeOrPackage(char[]) line: 178
ClassScope(Scope).getTypeOrPackage(char[], int) line: 2317
ClassScope(Scope).getType(char[]) line: 2057
SingleTypeReference.getTypeBinding(Scope) line: 44
SingleTypeReference(TypeReference).resolveType(ClassScope) line: 159
SingleTypeReference(TypeReference).resolveSuperType(ClassScope) line: 110
ClassScope.findSupertype(TypeReference) line: 1106
ClassScope.connectSuperclass() line: 826
ClassScope.connectTypeHierarchy() line: 951
CompilationUnitScope.connectTypeHierarchy() line: 290
LookupEnvironment.completeTypeBindings() line: 197
CompilationUnitResolver(Compiler).beginToCompile(ICompilationUnit[]) line: 387
CompilationUnitResolver.resolve(CompilationUnitDeclaration, ICompilationUnit, NodeSearcher, boolean, boolean, boolean) line: 836
CompilationUnitResolver.resolve(ICompilationUnit, IJavaProject, NodeSearcher, Map, WorkingCopyOwner, boolean, ...) line: 510
ASTParser.internalCreateAST(IProgressMonitor) line: 835
ASTParser.createAST(IProgressMonitor) line: 628
ASTProvider$1.run() line: 605
SafeRunner.run(ISafeRunnable) line: 37
ASTProvider.createAST(IJavaElement, IProgressMonitor) line: 600
ASTProvider.getAST(IJavaElement, ASTProvider$WAIT_FLAG, IProgressMonitor) line: 514
JavaEditor$8.run(IProgressMonitor) line: 3250
Worker.run() line: 58

Thread [Worker-3] (Suspended (breakpoint at line 554 in ClassFileReader))
ClassFileReader.getSourceName() line: 554
BinaryTypeBinding.<init>(PackageBinding, IBinaryType, LookupEnvironment) line: 150
LookupEnvironment.createBinaryTypeFrom(IBinaryType, PackageBinding, boolean, AccessRestriction) line: 560
LookupEnvironment.createBinaryTypeFrom(IBinaryType, PackageBinding, AccessRestriction) line: 557
HierarchyResolver.accept(IBinaryType, PackageBinding, AccessRestriction) line: 99
LookupEnvironment.askForType(char[][]) line: 101
UnresolvedReferenceBinding.resolve(LookupEnvironment, boolean) line: 43
BinaryTypeBinding.resolveType(ReferenceBinding, LookupEnvironment, boolean) line: 97
BinaryTypeBinding.superclass() line: 909
HierarchyResolver.resolve(IGenericType) line: 528
IndexBasedHierarchyBuilder(HierarchyBuilder).buildSupertypes() line: 115
IndexBasedHierarchyBuilder.build(boolean) line: 134
TypeHierarchy.compute() line: 300
TypeHierarchy.refresh(IProgressMonitor) line: 1237
CreateTypeHierarchyOperation.executeOperation() line: 90
CreateTypeHierarchyOperation(JavaModelOperation).run(IProgressMonitor) line: 720
CreateTypeHierarchyOperation(JavaModelOperation).runOperation(IProgressMonitor) line: 779
BinaryType.newSupertypeHierarchy(WorkingCopyOwner, IProgressMonitor) line: 793
BinaryType.newSupertypeHierarchy(IProgressMonitor) line: 745
SuperTypeHierarchyCache.getTypeHierarchy(IType, IProgressMonitor) line: 120
SuperTypeHierarchyCache.getTypeHierarchy(IType) line: 80
SuperTypeHierarchyCache.getMethodOverrideTester(IType) line: 89
OverrideIndicatorLabelDecorator.getOverrideIndicators(IMethod) line: 162
OverrideIndicatorLabelDecorator.computeAdornmentFlags(Object) line: 129
OverrideIndicatorLabelDecorator.decorate(Object, IDecoration) line: 261
LightweightDecoratorDefinition.decorate(Object, IDecoration) line: 259
LightweightDecoratorManager$LightweightRunnable.run() line: 71
SafeRunner.run(ISafeRunnable) line: 37
Platform.run(ISafeRunnable) line: 850
LightweightDecoratorManager.decorate(Object, DecorationBuilder, LightweightDecoratorDefinition) line: 336
LightweightDecoratorManager.getDecorations(Object, DecorationBuilder) line: 322
DecorationScheduler$1.ensureResultCached(Object, boolean, IDecorationContext) line: 357
DecorationScheduler$1.run(IProgressMonitor) line: 322
Worker.run() line: 58

Comment 11 Kent Johnson 2006-11-30 15:59:49 EST

We 3 choices to fix this problem:

1. Jerome -> change your cache to hold onto the class file bytes instead of the ClassFileReader instances. ClassFileReader was never designed to be thread safe.

2. Or set each accessor that uses lazy initialization to be a synchronized method.

3. Or fix the problem for #getSourceName() by changing it to [and any other accessors]:

public char[] getSourceName() {
  if (this.sourceName != null) 
    return this.sourceName;
  
  char[] name = getInnerSourceName(); // member or local scenario
  if (name == null) {
    name = getName(); // extract from full name
    int start;
    if (isAnonymous()) {
      start = CharOperation.indexOf('$', name, CharOperation.lastIndexOf('/', name) + 1) + 1;      
    } else {
      start = CharOperation.lastIndexOf('/', name) + 1;      
    }
    if (start > 0) {
      name = new char[name.length - start];
      System.arraycopy(name, start, name, 0, name.length);
    }
    this.sourceName = name;
  }
  return this.sourceName;  
}


My preference is #1

Comment 12 Kent Johnson 2006-12-01 13:53:54 EST

The other potential fix is to pre-initialize all lazy fields when fullyInitialize is set to true (which it is in this case).

This would mean the cached ClassFileReader instance would basically be a read-only object.

Any other choices? Any opinions on which one we should do?

Comment 13 Jerome Lanneluc 2006-12-04 11:03:30 EST

As discussed in JDT Core call, we should do option #3, which makes it thread safe and avoids deadlocks.

Comment 14 Kent Johnson 2006-12-04 15:14:39 EST

Changed getSourceName() as described in comment #11.

To reproduce, follow the steps in comment #9.

Comment 15 Kent Johnson 2006-12-04 15:15:38 EST

Released for 3.3 M4 in HEAD stream

Comment 16 David Audel 2006-12-12 11:08:30 EST

Verified for 3.3M4 with I20061212-0010.

Comment 17 Krzysztof Daniel 2008-10-09 09:56:28 EDT

Created attachment 114678 [details]
Backport of the fix to 3.2.2