Bugzilla – Full Text Bug Listing
| Summary: | DebugPlugin.start() generates NPE if activated early in startup | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Terry Parker <tparker> | ||||||||||
| Component: | Resources | Assignee: | Szymon Ptaszkiewicz <sptaszkiewicz> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||
| Severity: | critical | ||||||||||||
| Priority: | P3 | CC: | daniel_megert, Michael_Rennie, pawel.1.piech, remy.suen, sptaszkiewicz, Szymon.Brandys | ||||||||||
| Version: | 3.8 | Flags: | Szymon.Brandys:
review+
|
||||||||||
| Target Milestone: | 3.8 M6 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 209790 [details]
Two projects that generate the NPE
The patch contains two workspaces, "efs" and "JavaWithEfsFilter", from which I removed the .metadata folders. Steps to reproduce the bug:
1) Launch Eclipse 3.8M4 or later
2) Open the "efs" workspace and import the two existing projects there.
3) Create a new Eclipse Application launch configuration and point it at the ${workspace_loc}/../JavaWithEfsFilter workspace. Launch it.
4) Import the single project under that workspace.
5) Open its Sample.java file and set a breakpoint.
6) Restart that workspace
Created attachment 209793 [details]
Two projects that generate the NPE
Uploaded the incorrect version of the projects, this version should reproduce the bug.
Thank you Terry for the awesome bundle to reproduce the problem. First things first, IMO we should back out this change from 3.7.2 (I'll create a separate bug for that), and 3.8M5. For 3.8M5, I would recommend using my proposed fix that doesn't trigger breakpoint manager loading at startup. Dani, Mike, do you agree? (In reply to comment #3) > Thank you Terry for the awesome bundle to reproduce the problem. Yes, thank you very much for the examples! > First things first, IMO we should back out this change from 3.7.2 (I'll create a separate > bug for that), and 3.8M5. For 3.8M5, I would recommend using my proposed fix > that doesn't trigger breakpoint manager loading at startup. Dani, Mike, do you > agree? The complete stacktrace shows jdt.core causing the NPE when we try to re-create the Java breakpoint, it also contains a lot of entries about jdt.core failing to load. The same jdt.core failures causes jdt.ui to fail as well with exceptions like: Caused by: java.lang.NoClassDefFoundError: org/eclipse/jdt/core/IBufferFactory at java.lang.Class.getDeclaredConstructors0(Native Method) at java.lang.Class.privateGetDeclaredConstructors(Unknown Source) at java.lang.Class.getConstructor0(Unknown Source) at java.lang.Class.newInstance0(Unknown Source) at java.lang.Class.newInstance(Unknown Source) at org.eclipse.osgi.framework.internal.core.AbstractBundle.loadBundleActivator(AbstractBundle.java:167) I have jdt.core in my workspace, so I'll give this a quick debug to see what is happening. Created attachment 209831 [details]
stacktrace
This is the complete stacktrace generated from the examples
(In reply to comment #3) > Thank you Terry for the awesome bundle to reproduce the problem. First things > first, IMO we should back out this change from 3.7.2 (I'll create a separate > bug for that), and 3.8M5. For 3.8M5, I would recommend using my proposed fix > that doesn't trigger breakpoint manager loading at startup. Dani, Mike, do you > agree? I disagree. This would just be a workaround. You could put some other completely valid code into debug.core.start() and it will break as soon as it accesses core.resources, e.g. Resource.getPersistentProperty(). However, debug.core is allowed to access this and this should not fail. The example has a fundamental flaw it has an EFS implementation that itself requires core.resources but core.resources is based on EFS. As your example shows, this might or might not work i.e. it was just pure luck that it worked so far. On the other hand, I think core.resources should move the creation of the alias manager to the end in Workspace.startup(IProgressMonitor) to avoid loading bundles before it initialized itself as much as possible. With this change the cyclic example works. This however, is not something I'd recommend to do in 3.7.2. *** Bug 369239 has been marked as a duplicate of this bug. *** (In reply to comment #6) > (In reply to comment #3) > > Thank you Terry for the awesome bundle to reproduce the problem. First things > > first, IMO we should back out this change from 3.7.2 (I'll create a separate > > bug for that), and 3.8M5. For 3.8M5, I would recommend using my proposed fix > > that doesn't trigger breakpoint manager loading at startup. Dani, Mike, do you > > agree? > > I disagree. This would just be a workaround. You could put some other > completely valid code into debug.core.start() and it will break as soon as it > accesses core.resources, e.g. Resource.getPersistentProperty(). However, > debug.core is allowed to access this and this should not fail. > > The example has a fundamental flaw it has an EFS implementation that itself > requires core.resources but core.resources is based on EFS. As your example > shows, this might or might not work i.e. it was just pure luck that it worked > so far. I agree that this cycle from EFS to core.resources should be avoided. It was pure luck that it worked previously. It is not unreasonable to make that a requirement for EFS implementations. That said, I can imagine there are other scenarios that cause the debug.core to get activated early in the platform initialization process. If debug.core activation continues to load breakpoints, the constraints of what can be accessed when loading breakpoints needs to be documented/enforced. > > On the other hand, I think core.resources should move the creation of the alias > manager to the end in Workspace.startup(IProgressMonitor) to avoid loading > bundles before it initialized itself as much as possible. With this change the > cyclic example works. This however, is not something I'd recommend to do in > 3.7.2. > That said, I can imagine there are other scenarios that cause the debug.core to
> get activated early in the platform initialization process. If debug.core
> activation continues to load breakpoints, the constraints of what can be
> accessed when loading breakpoints needs to be documented/enforced.
This is not needed. It doesn't (or better it must not) matter whether it gets loaded early or late.
(In reply to comment #9) > > That said, I can imagine there are other scenarios that cause the debug.core to > > get activated early in the platform initialization process. If debug.core > > activation continues to load breakpoints, the constraints of what can be > > accessed when loading breakpoints needs to be documented/enforced. > > This is not needed. It doesn't (or better it must not) matter whether it gets > loaded early or late. I was discussing what might happen loading CDT breakpoints with Sergey, and breakpoints set on methods require access to the AST, which iiuc requires quite a bit of the framework to be initialized. It might be better to move breakpoint loading out of the activator if the debug.core plug-in needs to be activatable at any time. Startup sequences in frameworks are always tricky--placing restrictions on what can be done in early-loading code is often required. The question here is where is the best place to put a restriction. (In reply to comment #10) > (In reply to comment #9) > > > That said, I can imagine there are other scenarios that cause the debug.core to > > > get activated early in the platform initialization process. If debug.core > > > activation continues to load breakpoints, the constraints of what can be > > > accessed when loading breakpoints needs to be documented/enforced. > > > > This is not needed. It doesn't (or better it must not) matter whether it gets > > loaded early or late. > > I was discussing what might happen loading CDT breakpoints with Sergey, and > breakpoints set on methods require access to the AST, which iiuc requires quite > a bit of the framework to be initialized. It might be better to move > breakpoint loading out of the activator if the debug.core plug-in needs to be > activatable at any time. Startup sequences in frameworks are always > tricky--placing restrictions on what can be done in early-loading code is often > required. The question here is where is the best place to put a restriction. This bug here is about avoiding cycles. If you have another (performance) problem then that should be in a separate bug. Note though, that even without the fix for 345298, the breakpoint manager was started during startup by Debug UI (just not by Debug Core). I would assume this applies to CDT as well since it's based on Debug UI. Hence the real time saver would be to not eagerly create ASTs on startup. (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > > That said, I can imagine there are other scenarios that cause the debug.core to > > > > get activated early in the platform initialization process. If debug.core > > > > activation continues to load breakpoints, the constraints of what can be > > > > accessed when loading breakpoints needs to be documented/enforced. > > > > > > This is not needed. It doesn't (or better it must not) matter whether it gets > > > loaded early or late. > > > > I was discussing what might happen loading CDT breakpoints with Sergey, and > > breakpoints set on methods require access to the AST, which iiuc requires quite > > a bit of the framework to be initialized. It might be better to move > > breakpoint loading out of the activator if the debug.core plug-in needs to be > > activatable at any time. Startup sequences in frameworks are always > > tricky--placing restrictions on what can be done in early-loading code is often > > required. The question here is where is the best place to put a restriction. > > This bug here is about avoiding cycles. If you have another (performance) > problem then that should be in a separate bug. Note though, that even without > the fix for 345298, the breakpoint manager was started during startup by Debug > UI (just not by Debug Core). I would assume this applies to CDT as well since > it's based on Debug UI. Hence the real time saver would be to not eagerly > create ASTs on startup. It would be good to find a solid fix for bug 345298 in 3.8, so I'm restarting the discussion. Bug 345298 is still marked as fixed and should be reopened and marked dependent on this bug. It is marked as a duplicate for 9 other bugs. To respond to Dani's latest comment, I wasn't raising a performance issue, but rather stating that if the breakpoint loading is done in the debug.core activator, that activation may occur when only a subset of services are available. If so then all BreakPoint implementers must be able to complete construction of breakpoints in that more limited environment. I bring that up because breakpoints can be added via extension points so implementers need to know what to expect. Is there any guarantee to be made about what BreakPoint implementers can count on? > It would be good to find a solid fix for bug 345298 in 3.8, so I'm restarting > the discussion. Bug 345298 is still marked as fixed and should be reopened and > marked dependent on this bug. No, it should not be reopened. The fix is good but triggered this problem here. > To respond to Dani's latest comment, I wasn't raising a performance issue, but > rather stating that if the breakpoint loading is done in the debug.core > activator, that activation may occur when only a subset of services are > available. Right, and this should be changed in a general way via this bug here. (In reply to comment #13) > > It would be good to find a solid fix for bug 345298 in 3.8, so I'm restarting > > the discussion. Bug 345298 is still marked as fixed and should be reopened and > > marked dependent on this bug. > No, it should not be reopened. The fix is good but triggered this problem here. > I had thought that the change was backed out of both 3.7.2 and 3.8M5, but perhaps that wasn't the case. > > > To respond to Dani's latest comment, I wasn't raising a performance issue, but > > rather stating that if the breakpoint loading is done in the debug.core > > activator, that activation may occur when only a subset of services are > > available. > Right, and this should be changed in a general way via this bug here. Created attachment 211476 [details]
Patch
Szymon, please review. Szymon, could you send the change via Gerrit? See bug 371767. (In reply to comment #17) > Szymon, could you send the change via Gerrit? See bug 371767. Done. You may now do the review using Gerrit: https://git.eclipse.org/r/#/c/5184/ The patch submitted via Gerrit. See http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=4ea40760b3b7859ea85ed7b2cdf3399e24c1fbe0 |
Build Identifier: I20111209-1447 The fix for 345298 introduced an NPE which causes the JDT to fail to load when the following configuration is in place: * A custom EFS scheme gets instantiated at startup in a Java project * The EFS file system initialization causes loading of another utility plug-in * The utility plug-in's activator registers an org.eclipse.debug.core.ILaunchesListener * The Java project contains a Java breakpoint The issue is that the DebugPlugin is getting loaded before the workbench is fully initialized (this configuration is probably not unique in that respect). Previously activation of the DebugPlugin didn't access the workbench, but fix for 345298 added a call to BreakpointManager.start(), which accesses the workbench to load breakpoints. I'll follow up with an attachment containing a workspace that demonstrates this bug under 3.8M4. Here is a trimmed error log: !ENTRY org.eclipse.osgi 4 0 2012-01-18 09:09:11.594 !MESSAGE An error occurred while automatically activating bundle org.eclipse.jdt.core (275). !STACK 0 org.osgi.framework.BundleException: Exception in org.eclipse.jdt.core.JavaCore.start() of bundle org.eclipse.jdt.core. at org.eclipse.osgi.framework.internal.core.BundleContextImpl.startActivator(BundleContextImpl.java:734) ... at org.eclipse.jdt.internal.debug.core.JDIDebugPlugin.start(JDIDebugPlugin.java:266) ... at org.eclipse.debug.internal.core.BreakpointManager.createBreakpoint(BreakpointManager.java:547) at org.eclipse.debug.internal.core.BreakpointManager.loadBreakpoints(BreakpointManager.java:262) at org.eclipse.debug.internal.core.BreakpointManager.initializeBreakpoints(BreakpointManager.java:451) at org.eclipse.debug.internal.core.BreakpointManager.getBreakpoints0(BreakpointManager.java:420) at org.eclipse.debug.internal.core.BreakpointManager.start(BreakpointManager.java:406) at org.eclipse.debug.core.DebugPlugin.start(DebugPlugin.java:682) ... at com.google.eclipse.common.core.CommonCorePlugin.start(CommonCorePlugin.java:95) ... at com.google.eclipse.filesystem.core.FilteredLocalFileSystem.<init>(FilteredLocalFileSystem.java:193) ... at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:124) 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:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:616) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:622) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:577) at org.eclipse.equinox.launcher.Main.run(Main.java:1410) at org.eclipse.equinox.launcher.Main.main(Main.java:1386) Caused by: java.lang.NullPointerException at org.eclipse.core.internal.resources.Resource.getPersistentProperty(Resource.java:1173) at org.eclipse.jdt.internal.core.JavaModelManager.loadVariablesAndContainers(JavaModelManager.java:3220) at org.eclipse.jdt.internal.core.JavaModelManager.startup(JavaModelManager.java:4975) at org.eclipse.jdt.core.JavaCore.start(JavaCore.java:5144) at org.eclipse.osgi.framework.internal.core.BundleContextImpl$1.run(BundleContextImpl.java:711) at java.security.AccessController.doPrivileged(Native Method) at org.eclipse.osgi.framework.internal.core.BundleContextImpl.startActivator(BundleContextImpl.java:702) ... 163 more Root exception: java.lang.NullPointerException at org.eclipse.core.internal.resources.Resource.getPersistentProperty(Resource.java:1173) at org.eclipse.jdt.internal.core.JavaModelManager.loadVariablesAndContainers(JavaModelManager.java:3220) at org.eclipse.jdt.internal.core.JavaModelManager.startup(JavaModelManager.java:4975) at org.eclipse.jdt.core.JavaCore.start(JavaCore.java:5144) ... Reproducible: Always Steps to Reproduce: Will attach a pair of workspaces that reproduce the problem with 3.8M4.