Community
Participate
Working Groups
The following tests are failing when the Program to Run is pointed to: Run a product -> org.eclipse.sdk.ide ClassFileTests#testWorkingCopy01 ClassFileTests#testWorkingCopy02 ClassFileTests#testWorkingCopy05 ClassFileTests#testWorkingCopy10 The failures don't occur when run as Headless application.
I was affected by this, too, so I did some delta debugging and this is the point where I see behavior starting to differ: DefaultWorkingCopyOwner.createBuffer(ICompilationUnit) line: 32 ClassFileWorkingCopy.openBuffer(IProgressMonitor, Object) line: 84 ClassFileWorkingCopy(CompilationUnit).buildStructure(OpenableElementInfo, IProgressMonitor, Map, IResource) line: 116 ClassFileWorkingCopy(Openable).generateInfos(Object, HashMap, IProgressMonitor) line: 258 ClassFileWorkingCopy(JavaElement).openWhenClosed(Object, IProgressMonitor) line: 518 BecomeWorkingCopyOperation.executeOperation() line: 38 BecomeWorkingCopyOperation(JavaModelOperation).run(IProgressMonitor) line: 728 BecomeWorkingCopyOperation(JavaModelOperation).runOperation(IProgressMonitor) line: 788 ClassFile.getWorkingCopy(WorkingCopyOwner, IProgressMonitor) line: 571 ClassFileTests.testWorkingCopy01() line: 1238 In createBuffer, in the good case we have no primaryBufferProvider and proceed to super.createBuffer(), in the bad case, however, we do have one, an instance of JavaPlugin$2, and that unfortunate instance starts creating a Document for the jar file! As a consequence later the content of the jar file (undecoded zip!) is fed verbatim into the SourceElementParser, with expectable havoc. So if the jdt.ui has already started things go awry. I no longer think that just the test is broken, but we seem to have a real bug here. Someone has to check if the working copy is a ClassFileWorkingCopy. Since that class is internal, jdt.ui won't be able to. So the easiest fix would be this one-line change in DefaultWorkingCopyOwner: if (this.primaryBufferProvider != null && !(workingCopy instanceof ClassFileWorkingCopy)) return this.primaryBufferProvider.createBuffer(workingCopy); I have no idea what else this would break, though.
Alternatively, would JDT/UI have a chance to determine if an ICompilationUnit actually represents a class file, so they can avoid creating a bogus Document for those? Perhaps by checking the underlying resource for a ".class" suffix?
Given that the tested behavior is effectively broken in the IDE and since the Object Teams test suite is constantly affected by this I'd like to help resolve this bug. I don't see how a DocumentAdapter for a ClassFileWorkingCopy could ever make any sense? So wouldn't it indeed be best if UI's JavaPlugin avoids creating these bogus DocumentAdapters? (see JavaPlugin.java:368). Question remains: how can UI determine if a ICompilationUnit represents a class file? What would be cheaper, adding isBinary() to ICompilationUnit or letting ClassFileWorkingCopy implement a new public marker interface IClassFileWorkingCopy or just IClassFile to check against?
Created attachment 201477 [details] sketch of the simplest fix I did briefly play with letting ClassFileWorkingCopy implement IClassFile, which looked convincing at first, but it seems the hierarchies of ICompilationUnit CompilationUnit ClassFileWorkingCopy and IClassFile ClassFile cannot easily be made more symmetric, so I gave up on that. However, the attached patch (combined Core & UI) seems to do the trick - using indeed a new marker interface IClassFileWorkingCopy.
The patch is a no go. At the API level a working copy is for CUs if someone calls org.eclipse.jdt.core.WorkingCopyOwner.createBuffer(ICompilationUnit) for something else then an ICompiltaionUnit then this is just wrong. If JDT Core does this internally, then JDT Core must also internally protect itself. Having said that: AFAIK only JDT Core tests are affected. Hence the simplest fix is to call WorkingCopyOwner.setPrimaryBufferProvider(null) during test setup.
(In reply to comment #5) > The patch is a no go. At the API level a working copy is for CUs if someone > calls org.eclipse.jdt.core.WorkingCopyOwner.createBuffer(ICompilationUnit) for > something else then an ICompiltaionUnit then this is just wrong. Please help me understand what exactly is wrong. The tests show that it is possible that the argument to createBuffer is a ClassFileWorkingCopy, which is a subtype of ICompilationUnit. When wrapped in a DocumentAdapter this causes a text buffer to be created from the binary file content (we see a jar), causing havoc down the road. Are you saying the existence of class ClassFileWorkingCopy as a subclass of CompilationUnit is a bug? Or is it wrong to pass such an instance to createBuffer()? (I don't see this prohibited by the API doc). Maybe we have an impossible/inconsistent API contract here? > If JDT Core > does this internally, then JDT Core must also internally protect itself. I.e., must ensure that instances of ClassFileWorkingCopy *never* leak to any client? In that case I wouldn't know how IClassFile.getWorkingCopy(WorkingCopyOwner, IProgressMonitor) should be implemented. > Having said that: AFAIK only JDT Core tests are affected. Hence the simplest > fix is to call WorkingCopyOwner.setPrimaryBufferProvider(null) during test > setup. I'm not sure why you think so. Do these tests perform anything that can never happen in real life? They look pretty unsuspicious to me.
The API expects an ICompilationUnit and the client can do what it wants with it and he definitely has not to expect that he gets some binary stuff. If internal code passes something to that API which then blows up, then it's an internal implementation problem. The inheritance in this case is implementation based which often causes trouble. Look for example at 'ISourceManipulation' which is implemented an ICU: it clearly states: > Common protocol for Java elements that support source code manipulations such > * as copy, move, rename, and delete. So - if JDT Core passes binary elements to such an interface then this is just wrong.
(In reply to comment #7) > The API expects an ICompilationUnit and the client can do what it wants with it > and he definitely has not to expect that he gets some binary stuff. Sorry, I was looking at various potential sources for inconsistencies. It seems I missed that all your concerns were just about the contract of WorkingCopyOwner.createBuffer(ICompilationUnit). Investigating further I see that it was bug 110160 where Jerome "Added API IClassFile#becomeWorkingCopy(IProblemRequestor, WorkingCopyOwner, IProgressMonitor) that returns an ICompilationUnit in working copy mode on the given class file." As part of the patch the first real implementation was added to class ClassFileWorkingCopy, incl. method openBuffer which calls IBuffer buffer = this.owner.createBuffer(this); Strictly speaking this line breaks API, but actually the whole notion of a ClassFileWorkingCopy is problematic, no? Question: have the split refactorings mentioned in bug 110160 ever been implemented? If so, which tests relate to that feature? I tried to fix this by avoiding the above call to createBuffer. I succeeded to make all of AllJavaModelTests pass which one exception: org.eclipse.jdt.core.tests.model.WorkingCopyTests.testOnClassFile() which explicitly tests use of a custom working copy owner for creating custom buffers for class files. This particular test is not related to any bug, as it is older than the eclipse CVS. So, how relevant is that test, anyone know any use cases diving into this pond? According to how I understand Dani, that test case cannot PASS without breaking an API contract, right?
Created attachment 201644 [details] alternative fix This is how we can avoid breaking the contract of createBuffer for the price of breaking org.eclipse.jdt.core.tests.model.WorkingCopyTests.testOnClassFile()
> Strictly speaking this line breaks API, but actually the whole notion > of a ClassFileWorkingCopy is problematic, no? Yes, this is due to the implementation inheritance. > Question: have the split refactorings mentioned in bug 110160 ever been > implemented? Yes, see bug 106207. The patch looks good to me but I would use IBuffer buffer = BufferManager.createBuffer(this);
Created attachment 202725 [details] improved fix OK, this patch adopts Dani's suggestion from comment 10 and disables the bogus test WorkingCopyTests.testOnClassFile(). It'd be cool if anyone new the answer to part two of this: > Question: have the split refactorings mentioned in bug 110160 ever been > implemented? If so, which tests relate to that feature? Anyway, all java model tests pass with this patch, even in the IDE :) Jay, what do you think?
(In reply to comment #11) > It'd be cool if anyone new the answer to part two of this: Pls. read phonetically: "new" -> "knew" :)
The tests are all passing. Perfect! Thanks Stephan and I am assigning the bug to you.
Released for 3.8 M2.
I cannot even start the tests in that mode. I am using a 4.2 I-build. I am getting: java.lang.IllegalArgumentException: Argument cannot be null at org.eclipse.swt.SWT.error(SWT.java:4264) at org.eclipse.swt.SWT.error(SWT.java:4198) at org.eclipse.swt.SWT.error(SWT.java:4169) at org.eclipse.swt.widgets.Widget.error(Widget.java:468) at org.eclipse.swt.widgets.Widget.checkParent(Widget.java:277) at org.eclipse.swt.widgets.Widget.<init>(Widget.java:149) at org.eclipse.swt.widgets.Control.<init>(Control.java:110) at org.eclipse.swt.widgets.Scrollable.<init>(Scrollable.java:75) at org.eclipse.swt.widgets.Composite.<init>(Composite.java:95) at org.eclipse.ui.splash.BasicSplashHandler.getBundleProgressMonitor(BasicSplashHandler.java:159) at org.eclipse.ui.internal.Workbench$3.run(Workbench.java:543) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:519) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:123) at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.runApp(NonUIThreadTestApplication.java:54) at org.eclipse.pde.internal.junit.runtime.UITestApplication.runApp(UITestApplication.java:41) at org.eclipse.pde.internal.junit.runtime.NonUIThreadTestApplication.start(NonUIThreadTestApplication.java:48) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:344) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:624) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:579) at org.eclipse.equinox.launcher.Main.run(Main.java:1431) at org.eclipse.equinox.launcher.Main.main(Main.java:1407)
In fact even UI tests don't run fine using that build. Investigating.
This is related to bug 357521. Once this is fixed, I should be able to verify this fix.
With a fix from Bodgan, I could finally start UI tests and verify this fix. I am surprised nobody noticed that UI tests don't start in a 4.2 workspace. Verified for 3.8M2.
