Bugzilla – Full Text Bug Listing
| Summary: | Deactivated Shell closed with a fast click causes "Widget is disposed" SWT Error. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [RT] RAP | Reporter: | Istvan Ballok <Istvan.Ballok> | ||||
| Component: | RWT | Assignee: | Project Inbox <rap-inbox> | ||||
| Status: | ASSIGNED --- | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | ||||||
| Version: | 1.4 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | 397590, 325756 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
Created attachment 181824 [details]
correction to the event reordering patch (Shell activate / close)
In fact, in this context, we found that the proposed fix is not enough.
Explanation
===========
The Shell#close method is called from the ShellLCA#readData, in the
PhaseId.READ_DATA phase.
Because firing events is not allowed in the READ_DATA phase, the
ProcessActionRunner#add method is used, passing a runnable that
creates and fires the event later in the PROCESS_ACTION phase.
e.g. In DisplayLCAFacade#doProcessAction, first all the runnables are
executed, and then the queued events, in the specified order.
All this happens in the PROCESS_ACTION phase.
This explains what the problem is with the Shell#close implementation:
public void close() {
checkWidget();
ProcessActionRunner.add( new Runnable() {
public void run() {
ShellEvent event
= new ShellEvent( Shell.this, ShellEvent.SHELL_CLOSED );
event.processEvent();
if( event.doit ) {
Shell.this.dispose();
}
}
} );
}
Being in the PROCESS_ACTION phase, "event.processEvent()" executes the
event at once, synchronously, and does not enqueue it.
In fact, probably this was the motivation to use a ProcessActionRunner
in this case - because, AFTER the event was executed, the doit flag
is checked and the instance is disposed. Because of this need,
the processEvent() need to be synchron.
This means that this event is executed ahead of his rank concerning
the event order!!
because it is executed together with the Process Action Runnable-s,
and before the queued and sorted events.
So, e.g., if in the same request, a ShellEvent.Activate was also sent,
(for the use case see the main article of the bug report), then the
order of these events is not correct:
1) SHELL_CLOSED
- first, the shell closed is fired together with the Process Action Runnables
//-> the shell is disposed
and then, together with the queued events, the ShellEvent:
2) SHELL_ACTIVATED is fired.
// Note: the shell might be already disposed at this point.
// -> exception
the order of these events should be the exact opposite.
The supplied patch is a straightforward and dirty workaround for
this problem.
Thanks for the detailed explanation of the problem. We will look on it as soon as possible together with bug 325756. The Istvan's explanation is perfectly correct. The problem is in Shell#close where the close event is wrapped in a ProcessActionRunner. This is needed to respect the veto on the shell close by setting the doit flag to false. Thus, the event order is skipped. ProcessActionRunner runnables are executed before the queued events. As a result close event is fired before the activate event. |
The Problem =========== Deactivated Shell closed with a fast click causes "Widget is disposed" SWT Error. How to reproduce ================ Add a Shell listener to the ShellTab of the RWT Controls demo and implement the shellActivated method: public void shellActivated( ShellEvent e ) { e.widget.getData(); } This demonstrates that the shell is accessed when this event is fired. This models the behaviour of org.eclipse.ui.internal.services.CurrentSelectionSourceProvider that caused the problem in our real life usecase. Open this shell in the demo, and place it beside the main shell, so that both are visible at the same time. Focus the main shell so that the newly opened shell loses focus. Close the newly opened shell with a fast click, either by clicking on the close symbol (x) or clicking the OK button. A "widget is disposed" exception is thrown. Explanation of the problem ========================== When closing an unfocused shell, either with the close symbol or with any other button, 2 events are fired: 1) the shell is activated ( = focused; org.eclipse.swt.events.shellActivated) 2) a) the shell is closed ( = the X symbol was clicked; org.eclipse.swt.widgets.Shell_close) 2) b) widget is selected ( = the button was clicked) 1) is fired on mouse down, whereas 2) a) and b) is fired on mouse up. in org.eclipse.swt.Request, sending a request is implemented with a delay of 60 ms. => If the interval between the mouse down and mouse up events is shorter than 60 ms (=fast click), the 2 events are sent in a single request. If the interval is longer, the 2 events are sent separately, and the bug does not happen. So, we consider the case when the 2 events are sent together. (fast click) a) the shell is closed with the X symbol. ----------------------------------------- the ShellLCA#readData method handles the event from the client side and creates SWT events for each of them, in this order: 1. shell closed (ShellEvent) 2. shell (de)activated (ShellEvent) 3. mouse event (MouseEvent) The org.eclipse.swt.events.TypedEvent#EVENT_ORDER defines that e.g. the mouse events are processed before the shell event. However, the shell closed and shell activated events are of the same type (ShellEvent), so that the event order is not specified for them and is dependent on the order of insertion. = first, the shell closed listeners are called, and after that, the shell activated listeners. The shell closed event causes the shell to be disposed, and the listeners of the shell activated event can not access the instance, because it is already disposed. (= explanation of the problem) Resolution of the problem a) ---------------------------- Change order of the event creation in the ShellLCA: the shell activated event must be queued before the shell closed event. @@ -74,15 +74,17 @@ public final class ShellLCA extends AbstractWidgetLCA { // Important: Order matters, readMode() before readBounds() readMode( shell ); readBounds( shell ); - if( WidgetLCAUtil.wasEventSent( shell, JSConst.EVENT_SHELL_CLOSED ) ) { - shell.close(); - } + processActiveShell( shell ); processActivate( shell ); ControlLCAUtil.processMouseEvents( shell ); ControlLCAUtil.processKeyEvents( shell ); ControlLCAUtil.processMenuDetect( shell ); WidgetLCAUtil.processHelp( shell ); + + if( WidgetLCAUtil.wasEventSent( shell, JSConst.EVENT_SHELL_CLOSED ) ) { + shell.close(); + } } b) widget is selected ( = the button was clicked) ------------------------------------------------- In this case, two separate events are fired, namely a ShellEvent and a SelectionEvent. In org.eclipse.swt.events.TypedEvent#EVENT_ORDER, it is specified, that the SelectionEvent is handled before the ShellEvent. Clicking the OK button closes and disposes the shell. Hence, when the ShellEvent/Activate listeners are called, the Shell is already disposed. Resolution of the problem a) ---------------------------- Here, a clear and correct solution is not so easy. Changing the ranking of the ShellEvent by placing it before the SelectionEvent is a simple workaround, but we are not aware of its consequences and not convinced that it solves the problem in all cases. (e.g., it is possible to register a MouseListener on the shell and close it on mouse down; that would motivate us to rank it even higher as the MouseEvent) The correct solution would probably be to ensure that the event order is preserved in the combined, multiple requests in the 60 ms timeframe. Or, as far as we could understand it, the ShellEvent (containing activate, deactivate, and close) should be ranked just as high as the ActivateEvent. That would solve this "concurrency" issue with the SelectionEvent and the possible issue with the MouseEvent. But, unfortunately, we do not have the overview to decide what the correct event order would be. A workaround in the application layer is to always check whether the widget at hand is already disposed. This is however not applicable when using RCP components (like workbench/CurrentSelectionSourceProvider).