Comment 1 Frederic Fusier 2009-04-23 04:31:52 EDT

Note that I also get these exceptions with previous I build I20090416-1053 (i.e. JDT/Core v_949), so it does not seem related to fix for bug 271102 as bug 273308 was...

Comment 2 Srikanth Sankaran 2009-04-24 05:26:30 EDT

(In reply to comment #0)
> While trying to get numbers for bug 273308, I got several NPEs while closing
> projects in my workspace:

Can you document the steps I should follow to reproduce this.
(I am not yet aware of how performance numbers are collected
to walk in your shoes) - Thanks,

Comment 3 Frederic Fusier 2009-04-24 05:31:46 EDT

(In reply to comment #2)
> (In reply to comment #0)
> > While trying to get numbers for bug 273308, I got several NPEs while closing
> > projects in my workspace:
> 
> Can you document the steps I should follow to reproduce this.
> (I am not yet aware of how performance numbers are collected
> to walk in your shoes) - Thanks,
> 
This test was not an JDT/Core perf test but simply close projects in a worskpace (typically a 3.5 full source workspace). I wanted to know if the regression was visible from a user standpoint...

Comment 4 Srikanth Sankaran 2009-04-27 05:42:55 EDT

(In reply to comment #3)

[...]
 
> This test was not an JDT/Core perf test but simply close projects in a
> worskpace (typically a 3.5 full source workspace). I wanted to know if the
> regression was visible from a user standpoint...

No, I don't get this in my 3.5 workspace, (I have only the JDT/Core projects
there - not sure if that is what you meant by "full source workspace").

I am trying to work backwards from the exception call stack now. It would
appear that the NPE would be generated if the delta associated with the
resourceChanged event is null but there are external elements to refresh.

I'll see how hard/easy it would be work from here to reproduction of NPE. 

Comment 5 Frederic Fusier 2009-04-27 06:01:59 EDT

(In reply to comment #4)
> 
> No, I don't get this in my 3.5 workspace, (I have only the JDT/Core projects
> there - not sure if that is what you meant by "full source workspace").
> 
Sorry, I should have explained my terminology... For me, a full source workspace is a brand new workspace in which I imported all the target platform plugins as 'Projects with source folders'...

Comment 6 Frederic Fusier 2009-04-27 07:13:27 EDT

Note that I can't reproduce in a newly created full source workspace. I guess there's something special in mine making the NPE happened. I'll debug it to likely give you more clues on this issue...

Comment 7 Michael Rennie 2009-06-02 16:39:23 EDT

I20090601-2000

I got the following exception closing 3 projects:

org.eclipse.ui
org.eclipse.ui.workbench
org.eclipse.ui.workbench.texteditor


java.lang.NullPointerException
at org.eclipse.jdt.internal.core.DeltaProcessor.checkProjectsAndClasspathChanges(DeltaProcessor.java:297)
at org.eclipse.jdt.internal.core.DeltaProcessor.resourceChanged(DeltaProcessor.java:1901)
at org.eclipse.jdt.internal.core.DeltaProcessingState.resourceChanged(DeltaProcessingState.java:470)
at org.eclipse.core.internal.events.NotificationManager$2.run(NotificationManager.java:291)
at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42)
at org.eclipse.core.internal.events.NotificationManager.notify(NotificationManager.java:285)
at org.eclipse.core.internal.events.NotificationManager.handleEvent(NotificationManager.java:249)
at org.eclipse.core.internal.resources.Workspace.broadcastEvent(Workspace.java:307)
at org.eclipse.core.internal.resources.Project.close(Project.java:165)
at org.eclipse.ui.actions.CloseResourceAction.invokeOperation(CloseResourceAction.java:218)
at org.eclipse.ui.actions.WorkspaceAction.execute(WorkspaceAction.java:162)
at org.eclipse.ui.actions.WorkspaceAction$2.runInWorkspace(WorkspaceAction.java:483)
at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:38)
at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)

Comment 8 Srikanth Sankaran 2009-06-03 01:12:25 EDT

(In reply to comment #7)
> I20090601-2000
> I got the following exception closing 3 projects:
> org.eclipse.ui
> org.eclipse.ui.workbench
> org.eclipse.ui.workbench.texteditor
> java.lang.NullPointerException

Do you have a reproducible scenario ? If so, can you provide a step by
step procedure leading to the NPE ?

Jerome, from what is described in comment #4, would you be able to cook
up a test case ? (I invested a couple of hours on this exercise and didn't
have much success)

Comment 9 Jerome Lanneluc 2009-06-03 04:47:14 EDT

(In reply to comment #8)
> Jerome, from what is described in comment #4, would you be able to cook
> up a test case ? (I invested a couple of hours on this exercise and didn't
> have much success)
Unfortunately, I don't know under what condition a POST_CHANGE event is fired with a null delta. John, would you know?

Comment 10 John Arthorne 2009-06-03 07:55:18 EDT

The delta is never null for a POST_CHANGE event. We check for a null delta before calling listeners, and just return. However, I can see from this stack trace that it is actually a PRE_CLOSE event, which always has a null delta.

On line 4622 of JavaModelManager there is code that hard-codes the event type to be POST_CHANGE, I wonder if this is causing the problem:

										JavaModelManager.this.deltaState.getDeltaProcessor().overridenEventType = IResourceChangeEvent.POST_CHANGE;

Then DeltaProcessor does this:

int eventType = this.overridenEventType == -1 ? event.getType() : this.overridenEventType;

Comment 11 Michael Rennie 2009-06-03 09:51:31 EDT

(In reply to comment #8)
> Do you have a reproducible scenario ? If so, can you provide a step by
> step procedure leading to the NPE ?
> 

Unfortunately no, I tried multiple times opening / closing the three projects that caused the problem (for me) but could not reproduce.

Comment 12 Srikanth Sankaran 2009-06-08 05:33:29 EDT

(In reply to comment #10)
> The delta is never null for a POST_CHANGE event. We check for a null delta
> before calling listeners, and just return. However, I can see from this stack
> trace that it is actually a PRE_CLOSE event, which always has a null delta.

Thanks for the clarification, John.

> On line 4622 of JavaModelManager there is code that hard-codes the event type
> to be POST_CHANGE, I wonder if this is causing the problem:

Indeed, as we recycle worker threads, we are inadveratantly referencing
stale data, which explains why we both Frederic and Michael had some
problem reproducing - the PRE_CLOSE event has to be delivered in the
same worker thread which was used to deliver the POST_BUILD event 
originating from the SavedState for the problem to reproduce (in addition
to there being external elements to refresh).

Patch will follow shortly.

Comment 13 Srikanth Sankaran 2009-06-08 07:09:50 EDT

Created attachment 138560 [details]
Proposed patch

No tests have been created, since this problem shows
up only under specific thread scheduling. Verify
by code inspection.

Comment 14 Srikanth Sankaran 2009-06-08 07:14:49 EDT

(In reply to comment #13)
> Created an attachment (id=138560) [details]
> Proposed patch
> No tests have been created, since this problem shows
> up only under specific thread scheduling. Verify
> by code inspection.

Jerome,

    A couple of questions:

    1. There is no action in org.eclipse.jdt.internal.core.DeltaProcessor.resourceChanged(IResourceChangeEvent) for PRE_CLOSE event. I am assuming this
is intentional and not an oversight. (I did notice that we
dispose of the resources via a call to JavaProject.close when a
subsequent POST_CHANGE event is posted).

    2. What is the background/history behind this org.eclipse.jdt.internal.core.DeltaProcessor.overridenEventType ?
If we wanted the same action in DeltaProcessor.resourceChanged for
POST_BUILD as for POST_CHANGE, why won't we make the POST_BUILD case
fall through to POST_CHANGE instead of this overridenEventType ?

Comment 15 Jerome Lanneluc 2009-06-09 12:03:59 EDT

(In reply to comment #14)
>     1. There is no action in
> org.eclipse.jdt.internal.core.DeltaProcessor.resourceChanged(IResourceChangeEvent)
> for PRE_CLOSE event. I am assuming this
> is intentional and not an oversight. (I did notice that we
> dispose of the resources via a call to JavaProject.close when a
> subsequent POST_CHANGE event is posted).
Actually, I'm wondering if we should not do the same thing as for PRE_DELETE, e.g. discard indexing jobs for the project, remember the project's roots, etc.

>     2. What is the background/history behind this
> org.eclipse.jdt.internal.core.DeltaProcessor.overridenEventType ?
> If we wanted the same action in DeltaProcessor.resourceChanged for
> POST_BUILD as for POST_CHANGE, why won't we make the POST_BUILD case
> fall through to POST_CHANGE instead of this overridenEventType ?
> 
It appears that this was introduced by the fix for bug 59937. However I don't remember the details. Maybe it was a quick hack at the end of the 3.0 release. But your suggestion to make the POST_BUILD case fall through to POST_CHANGE seems appealing.

Comment 16 Srikanth Sankaran 2009-06-11 02:28:10 EDT

(In reply to comment #15)
> (In reply to comment #14)
> >     1. There is no action in
> > org.eclipse.jdt.internal.core.DeltaProcessor.resourceChanged(IResourceChangeEvent)
> > for PRE_CLOSE event. I am assuming this
> > is intentional and not an oversight. (I did notice that we
> > dispose of the resources via a call to JavaProject.close when a
> > subsequent POST_CHANGE event is posted).
> Actually, I'm wondering if we should not do the same thing as for PRE_DELETE,
> e.g. discard indexing jobs for the project, remember the project's roots, etc.

I have raised bug #279897 to investigate this.

> >     2. What is the background/history behind this
> > org.eclipse.jdt.internal.core.DeltaProcessor.overridenEventType ?
> > If we wanted the same action in DeltaProcessor.resourceChanged for
> > POST_BUILD as for POST_CHANGE, why won't we make the POST_BUILD case
> > fall through to POST_CHANGE instead of this overridenEventType ?
> > 
> It appears that this was introduced by the fix for bug 59937. However I don't
> remember the details. Maybe it was a quick hack at the end of the 3.0 release.
> But your suggestion to make the POST_BUILD case fall through to POST_CHANGE
> seems appealing.

On second thoughts, this will prove problematic. The current behavior is
to override only the the POST_BUILD originating from the SavedState with
a POST_CHANGE event and the proposed patch retains the behavior while
also fixing the NPE due to stale thread local state being accessed. 

(Making the POST_BUILD case fall through to POST_CHANGE would introduce
new behavior to every subsequent)

So, I'll retain the proposed the patch itself for release to 3.5.1. 
   

Comment 17 Srikanth Sankaran 2009-06-16 01:28:39 EDT

Olivier, I am looking at this as a candidate for 3.5.1. This is a crash
reported by two different people.

Comment 18 Olivier Thomann 2009-06-16 10:12:24 EDT

+1

Comment 19 Olivier Thomann 2009-06-22 09:48:52 EDT

+1.

Comment 20 Srikanth Sankaran 2009-06-23 01:45:33 EDT

Released for 3.5.1 Maintenence branch.
Released in HEAD for 3.6M1.

Comment 21 Srikanth Sankaran 2009-06-25 05:11:55 EDT

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

Comment 22 Frederic Fusier 2009-08-04 07:21:27 EDT

Verified for 3.6M1 using build 20090802-2000.

Comment 23 Olivier Thomann 2009-08-27 12:13:05 EDT

Verified for 3.5.1RC2 using M20090826-1100.