Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 292108

Summary:

Unguarded access to static map in ThemeManager could lead to application crash

Product:

[RT] RAP

Reporter:

Stefan Röck <stefan.roeck>

Component:

RWT

Assignee:

Project Inbox <rap-inbox>

Status:

RESOLVED FIXED

QA Contact:

Severity:

blocker

Priority:

Version:

1.3

Target Milestone:

1.3 M3

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Testcase for performance impacts	none

Description Stefan Röck

2009-10-13 02:57:03 EDT

org.eclipse.rwt.internal.theme.ThemeManager holds a map "adapters" that is not thread-safe. In ThemeManager#getThemeAdapter() this map is changed via calls from UIThreads. As these map operations are not thread-safe an inconsistent state could occur (especially when the map is dynamically resized internally).
In our case,  the following exceptions was thrown:
java.lang.IllegalArgumentException: No theme adapter registered for class org.eclipse.swt.widgets.Label
	at org.eclipse.rwt.internal.theme.ThemeManager.getThemeAdapter(ThemeManager.java:489)
Once, the ThemeAdapter was lost, this exceptions arose in all UIThreads and new sessions.

Comment 1 Ralf Sternberg

2009-10-13 05:01:05 EDT

Thanks for pointing this problem out, it must have been hard to find!

Apart from the problematic modification of the adapters map in ThemeManager#getThemeAdapter, the map access doesn't need to be synchronized because the theme adapters are registered at application startup and the map is not modified after initialization.

Since this map modification in ThemeManager#getThemeAdapter is a questionable optimization anyway, I leave this line of code out instead of synchronizing map access. The fix is in CVS HEAD.

Comment 2 Stefan Röck

2009-10-13 05:10:06 EDT

Created attachment 149421 [details]
Testcase for performance impacts

I wrote a test to measure the performance impacts.
Removing the put-call to the map leads to approx. 30% longer runtime of ThemeManager#getThemeAdapter. As this method is called quite often, this might be worth some investigation.
Using Collections.synchronizedMap() maybe helps. However, I'm satisfied with the proposed fix ;-)

Comment 3 Ralf Sternberg

2009-10-15 05:29:10 EDT

Yes you may be right. But since the performance penalty affects only custom widgets, and synchronization also affects performance, I was not convinced that synchronizing the map access is necessarily better. I think only a profiler can tell us whether this change has a real performance impact or not.

However, theme adapters should rather be resolved using an adapter factory (see bug 292362). So we should look into performance after this change.

Comment 4 Stefan Röck

2009-10-15 05:37:08 EDT

This affects all Widgets that are inherited from Control or Composite. If you mean those with "Custom widgets", I agree. I made some performance investigations using JProfiler with the result that leaving the "put" or adding synchronization around the "put" has nearly the same impact. 
However, of course this depends on how often the map is changed, how many threads are accessing, etc.

Comment 5 Ralf Sternberg

2009-10-15 05:50:27 EDT

(In reply to comment #4)
> This affects all Widgets that are inherited from Control or Composite. If you
> mean those with "Custom widgets", I agree.
No this should only affect widgets that have not registered a theme adapter of their own.

Comment 6 Stefan Röck

2009-10-15 06:37:22 EDT

I think this affects all inherited widgets whose base widgets get a ThemeAdapter. For instance in Control#getBackground() the ThemeAdapter for the current control class is fetched, which applies for all inherited classes for Control. The same applies for Shell (Shell#getShellThemeAdapter()) although its not recommended to subclass Shell.