> #3339 NPE caused by null defaultImage in XtextEditorTickUpdater (updateEditorImage)

[mehmet-karaman]

Set fields to volatile which could possible be accessed by multiple threads. Used local variables to store the value and added check with local variable.

[cdietrich]

can you also have a look at the subclasses

[mehmet-karaman]

There is only one subclass (XtendEditorErrorTickUpdater), which has only one field and that is not set to null..

[mehmet-karaman]

Ah .. I've got it.. Classes which implement this interface IXtextEditorCallback :)

[github-actions]

Test Results

  6 461 files  ±0    6 461 suites  ±0   3h 21m 31s ⏱️ + 3m 32s
 43 241 tests ±0   42 657 ✅ ±0    584 💤 ±0  0 ❌ ±0 
170 051 runs  ±0  167 714 ✅ ±0  2 337 💤 ±0  0 ❌ ±0 

Results for commit fc5f46d. ± Comparison against base commit d3d2755.

♻️ This comment has been updated with latest results.

[mehmet-karaman]

Found one more case, where this could happen.. OverrideIndicatorModelListener uses the editor, which is unset and its not volatile.. I've also used it as local variable to avoid setting to null afterwards..

[cdietrich]

no i mean the xtend one. do we need the local stuff you do there too

[mehmet-karaman]

Oh now I looked again, the changes in the "OverrideIndicatorModelListener" are not necessary, because the field is accessed only in the UI thread.. So no worker thread should execute this code..

The problem happens only if the UI thread calls the beforeDispose, meanwhile a worker thread accesses the fields which are set by the beforeDispose method to null. If you check the stacktrace in the ticket, you will see that the defaultImage is accessed by the worker thread, where the UI thread already set this defaultImage to null..

[mehmet-karaman]

We can keep the changes.. Maybe there is a worker thread which does the same for the next time.. So It shouldn't harm to keep these changes..

[mehmet-karaman]

I've done a rebase.

[iloveeclipse]

@mehmet-karaman : could you please check what is wrong with your commit metadata (autor mail seem not match one expected by ECA)?

[mehmet-karaman]

eclipsefdn/eca problem seems to be fixed.

[mehmet-karaman]

@cdietrich are we done here?

[cdietrich]

i have no time to try

[LorenzoBettini]

I'm not very familiar with this part of the code base.
If that's related to a bug, I can try to reproduce the bug as long as I'm given the detailed steps and verify that this PR fixes the bug.
Otherwise, @szarnekow may judge better than I do.
In general, I think the PR is OK.

[iloveeclipse]

Originally this bug was reported during our automated tests and from the stack trace it was almost obvious why that happened, since the test opens/closes Xtext editor and the background job simply runs after editor was already disposed.

I haven't tried, but to reproduce one should create breakpoint at the job code (XtextEditorErrorTickUpdater.modelChanged()) that runs while changing some text in Xtext based editor, change the text, close the editor and let the job continue. If that doesn't sow the problem, the text should be changed outside of the editor so we have a resource change.

IMHO the code was buggy and the fix is more or less trivial.