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

Bug 372960

Summary:

Sometimes CurrentPhase#get() returns a wrong phase

Product:

[RT] RAP

Reporter:

Ivan Furnadjiev <ivan>

Component:

RWT

Assignee:

Project Inbox <rap-inbox>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

Version:

1.4

Target Milestone:

1.5 M6

Hardware:

All

OS:

All

Whiteboard:

Bug Depends on:

Bug Blocks:

369521

Attachments:

Description	Flags
Proposed patch	none
Updated patch	none
Patch that implements the idea in comment#4	none

Description Ivan Furnadjiev

2012-03-01 06:10:15 EST

When debugging the issue in bug 369521 I found a strange behavior. The BrowserHistory fire navigation event in a wrong phase - READ_DATA instead of PROCESS_ACTION. But how this is possible when the event is fired after the check "event.getPhaseId() == PhaseId.PROCESS_ACTION"??!! After debugging I found that the event has the correct phaseId, but CurrentPhase#get() still returns the previous phase.

Comment 1 Ivan Furnadjiev

2012-03-01 06:24:09 EST

Created attachment 211867 [details]
Proposed patch

The problem is that CurrentPhase is using a phase listener too. It sets the current phase in beforePhase. That's why the order of execution of registered phase listener is crucial - the phase listener for the CurrentPhase *must* be executed first to set the correct current phase. The CurrentPhase phase listener is *always* registered in the PhaseListenerManager first - ApplicationContextConfigurator#addInternalPhaseListeners, but the order of execution of phased listeners is not guarantee by the PhaseListenerManager as HashSet is used to keep the listeners. The proposed patch changes the HashSet to LinkHashSet to ensure that phase listeners are executed in the order of which they are added.

Comment 2 Ivan Furnadjiev

2012-03-01 09:33:30 EST

Created attachment 211881 [details]
Updated patch

In addition to the previous patch HashSet is replaced with LinkHashSet in PhaseListenerRegistry too. JUnit tests for both classes added too.

Comment 3 Ivan Furnadjiev

2012-03-01 10:02:54 EST

It seems that this patch is not enough as phase listeners form the extension point (WorkbenchApplicationConfigurator) are still registered before the CurrentPhase listener - see ApplicationContextConfigurator#configure.

Comment 4 Ivan Furnadjiev

2012-03-01 11:21:45 EST

Just come to my mind another approach - CurrentPhase could not rely on a phase listener by set current phase explicitly in RWTLifeCycle or SimpleLifeCycle.

Comment 5 Ivan Furnadjiev

2012-03-01 17:38:20 EST

Created attachment 211919 [details]
Patch that implements the idea in comment#4

Comment 6 Ivan Furnadjiev

2012-03-02 01:35:24 EST

Applied last patch to CVS HEAD. CurrentPhase does not use phase listener anymore.