Bug 369133

Comment 1 Paul Webster 2012-01-20 08:45:15 EST

Were there any other errors reported in their error log?  This is usually a symptom of a java exception thrown from isEnabled() or a property tester.

PW

Comment 2 Brian Vosburgh 2012-09-19 14:41:12 EDT

I was getting this same stack trace every time I tried to open a new perspective in a new window (either via "Window > New Window" or "Window > Open Perspective > Java"). There were no other errors in my log. When I restarted Eclipse, the 3 windows that had failed to open actually showed up, and the StackOverflowErrors ceased.

Comment 3 Brian Vosburgh 2012-09-19 14:43:02 EDT

Sorry for the noise; but I forgot to mention I was using 3.8/Juno. [4.2/Juno just isn't cutting it for me yet. :-) ]

Comment 4 Paul Webster 2012-09-20 08:50:51 EDT

I think this is a transient state caused by partial processing of a context change.  That's one reason why it's hard to reproduce.

Brian, can you think of what was happening just before the problem started (I know it's hard to think back to your IDE use :-) ?

PW

Comment 5 Brian Vosburgh 2012-09-24 12:24:16 EDT

Created attachment 221427 [details]
stack overflow and preceding stack traces

Comment 6 Brian Vosburgh 2012-09-24 12:24:40 EDT

(In reply to comment #4)
> Brian, can you think of what was happening just before the problem started
> (I know it's hard to think back to your IDE use :-) ?

Well, I could not remember what I was doing last time; but I started up my workspace this morning, pulled some commits from our Git repository, and directly encountered this problem (comment 2) again. Attached is the log file, indicating that some other problems did, indeed, happen just beforehand.

Comment 7 Paul Webster 2013-12-13 10:12:46 EST

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

Comment 8 liang jia 2014-02-18 05:51:05 EST

Hi Eddie Galvez

I encountered the same java stack error, but I don't know how to reproduce it.
because this bug only can reproduce in our customer's env.

So could you please give me you detail steps to reproduce this bug or
give me some sample code to reproduce it?

Thanks.

Comment 9 liang jia 2014-02-19 03:12:40 EST

Hi All

I made a more deep investigate for the java stack,
the reason which cause the StackOverflowError is prefixTable 
variable is null(org.eclipse.jface.bindings.BindingManager.java)

but I checked the assignment location, it's can not be null,
There is no way to set prefixTable to null, exception below method in BindingManager:
private final void clearSolution() {
    setActiveBindings(null, null, null, null);
}

so my question is that who and when set the variable(prefixTable) to null? 

Does anyone has any suggestion or comments.

Thanks

Comment 10 liang jia 2014-02-19 03:40:48 EST

Hi All

I think we can using below code to fix this bug, 
although I still do not know how to reproduce this bug.

Change the setActiveBindings method in BindingManager.java
as below:
	private final void setActiveBindings(final Map activeBindings,
			final Map activeBindingsByCommandId, final Map prefixTable,
			final Map conflicts) {
		...
		if(prefixTable == null) {
			this.prefixTable = Collections.EMPTY_MAP;
		} else {
			this.prefixTable = prefixTable;
		}
		
		...
	}


PS: maybe getPrefixTable method in BindingManager.java also need change to:
	private final Map getPrefixTable() {
//		if (prefixTable == null) {
//			recomputeBindings();
//		}

		return prefixTable;
	}

Sincerely
Liang

Comment 11 Paul Webster 2014-02-20 14:57:58 EST

Hi Liang,

(In reply to liang jia from comment #10)
> 		if(prefixTable == null) {
> 			this.prefixTable = Collections.EMPTY_MAP;
> 		} else {
> 			this.prefixTable = prefixTable;
> 		}

This implies that the EMPTY_MAP is being returned for the prefix table, which will throw an exception if anything is put into it.

> 
> PS: maybe getPrefixTable method in BindingManager.java also need change to:
> 	private final Map getPrefixTable() {
> //		if (prefixTable == null) {
> //			recomputeBindings();
> //		}

This will prevent recomputing the bindings in some scenarios without considering where it was reset to null so it needs to be recomputed.

PW

Comment 12 liang jia 2014-02-20 20:14:14 EST

(In reply to Paul Webster from comment #11)
Hi Paul

Thanks for you reply.


> Hi Liang,
> 
> (In reply to liang jia from comment #10)
> > 		if(prefixTable == null) {
> > 			this.prefixTable = Collections.EMPTY_MAP;
> > 		} else {
> > 			this.prefixTable = prefixTable;
> > 		}
> 
> This implies that the EMPTY_MAP is being returned for the prefix table,
> which will throw an exception if anything is put into it.
> 
> > 
> > PS: maybe getPrefixTable method in BindingManager.java also need change to:
> > 	private final Map getPrefixTable() {
> > //		if (prefixTable == null) {
> > //			recomputeBindings();
> > //		}
> 
> This will prevent recomputing the bindings in some scenarios without
> considering where it was reset to null so it needs to be recomputed.
I checked the java stack, this java out of stack error is caused by prefixTable is null, and also I checked the BindingManager.java, the prefixTable never can be null. so my question is that when prefixTable can be null?
> 
> PW

Comment 13 Paul Webster 2014-02-20 20:21:26 EST

(In reply to liang jia from comment #12)
> I checked the java stack, this java out of stack error is caused by
> prefixTable is null, and also I checked the BindingManager.java, the
> prefixTable never can be null. so my question is that when prefixTable can
> be null?

It can be set from org.eclipse.jface.bindings.BindingManager.clearSolution()

PW

Comment 14 Eclipse Genie 2015-07-07 10:08:53 EDT

New Gerrit change created: https://git.eclipse.org/r/51496

Comment 15 Andrey Loskutov 2015-07-07 10:09:48 EDT

We also observe this on 3.8.2 and looking at the code of BindingManager at head of 4.6, this problem must be also still in 4.x stream.

I think this is a multi-threading issue, and coming from the fact that recomputeBindings() in one thread can read not fully initialized "existingCache" instance which is put to cachedBindings by the different thread.

I've pushed patch https://git.eclipse.org/r/51496.

Comment 16 Brian de Alwis 2015-07-07 11:11:50 EDT

Andrey, when are we recomputing the bindings on different threads?

Comment 17 Andrey Loskutov 2015-07-07 11:23:21 EDT

(In reply to Brian de Alwis from comment #16)
> Andrey, when are we recomputing the bindings on different threads?

Honestly I don't know but because there can not be any other explanation (or I do not see it) this can only be the reason for partially filled CachedBindingSet.

We observing this sporadically in our regression tests where the same editor opened/closed few times so I guess this could be also some bad client code involved (unfortunately we can't see the originating method in the stack and also can't reproduce this without tests).

Comment 18 Brian de Alwis 2015-07-07 11:31:03 EDT

It looks to me that moving the 'existingCache.setPrefixTable(prefixTable);' to before the setActiveBindings() call (which recurs in the stack trace above) is the key: we may have been whacking the prefix table with stale data.

Comment 19 Andrey Loskutov 2015-07-07 11:41:39 EDT

(In reply to Brian de Alwis from comment #18)
> It looks to me that moving the 'existingCache.setPrefixTable(prefixTable);'
> to before the setActiveBindings() call (which recurs in the stack trace
> above) is the key: we may have been whacking the prefix table with stale
> data.

This is posible too, but probably it would be visible more often? I suspect that someone manages to call the code from non UI thread - this would explain why we have only few reports.

Comment 20 Brian de Alwis 2015-07-07 12:00:19 EDT

I doubt we have binding-related code called off-thread.  My suspicion is that we rarely see prefix changes, so the stale table isn't actually stale.

Comment 21 Brian de Alwis 2015-07-08 21:42:41 EDT

Thanks Andrey!