Invalid lifecycle for ReactDialog

[CamilleLetavernier]

Bug Description:

In #11669, a fix was provided to support re-opening a ReactDialog that had been closed. However, trying to reopen/reuse a widget that has been previously disposed violates the widget API, which may cause further issues. Especially, BaseWidget.dispose() can only be invoked once, so the reused/revived widget can no longer be disposed after this.

As a consequence, dialogs based on ReactDialog now have an inconsistent lifecycle, which causes errors:

ERROR Warning: You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it.

My understanding is that we shouldn't be trying to reuse closed (disposed) Dialogs in the first place; instead, a new instance of the dialog should be created after the previous one was closed/disposed.

Steps to Reproduce:

1. In the Theia Browser Example, open the AboutDialog
2. Close it using Esc key or X button: Dialog is properly disposed
3. Reopen it: Dialog is "successfully" reused, but is still flagged as disposed
4. Close it again: Dispose is not properly invoked, because the widget is already disposed
5. Reopen it once more: A React error is thrown, because we're trying to mount the dialog again, even though it wasn't successfully unmounted in the previous step

ERROR Warning: You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before. Instead, call root.render() on the existing root instead if you want to update it.

Additional Information

• Operating System: Any
• Theia Version: Tested on 1.33.0, but probably any version >= 1.30.0

[kittaakos]

We have also noticed this after switching to Theia 1.31.1 and had to patch all the dialogs in the Theia app. For the time being, if anybody is interested in a workaround: arduino/arduino-ide@ac9cce1#diff-e9b03ff2d6226d1a82f1134e027997f6963315674476dfc2a6abcc7197891ede

[MatthieuBarzellinoSTM]

Thanks for the workaround @kittaakos!

[noneghost]

after call addAcceptButton click accept close dialog, the dispose no called.

[tsmaeder]

@CamilleLetavernier what are you suggesting should be done here? That we stop reusing dialogs in the Theia framework or that the lifecycle of dialogs be fixed?

[tsmaeder]

We seem to have a conceptual problem here: BaseWidget.close() invokes Widget.dispose() and thus makes all dialogs not reusable. There are a couple of approaches to fixing this one:

1. Don't use dialog singletons: this respects the semantics of base widget. Clients would have to bind a factory instead of the dialog instance
2. Do not call super.onClose() in AbstractDialog.onClose(). This would prevent the double dispose in the about dialog, but we would have to udpate all clients of AbstractDialog to dispose of the widgets themselves. By adding an optional dispose: boolean ? true parameter to AbstractDialog.show(), we might limit the number of changes clients need to do.
3. Change BaseWidget.close() to not invoke dispose() upon widget close. Clients closing widgets would have to invoke dispose() on the widget when they are done with it.

I believe 3. is the only correct way, and here's why: when a client invokes ApplicationShell.addWidget(), it can pass widgets which are not derived from BaseWidget. When we later call ApplicationShell.closeWidget() on that widget, we should invoke dispose() on the widget after calling close() on it, since we do not know that close() invokes dispose(). Currently, any widget that does not dispose itself upon close() will go undisposed and might leak resources. However, calling dispose() on a BaseWidget after it is close will double-dispose and throw an error.
That said, this change would be quite fundamental breaking change and we'd have to make sure we invoke dispose() in all the right places. My suggestion therefore is to implement 2. above as an immediate fix and to open a separate issue about the close/dispose refactoring.

[sdirix]

I don't have hard any hard feelings in this topic. However it seems a bit weird to me that we have two fundamentally different concepts for widgets and dialogs although dialogs are also just widgets.

Therefore I would tend to unify them by refactoring dialogs to NOT being singletons.

[tsmaeder]

@CamilleLetavernier I can only reproduce the problem with the about dialog in the browser version of Theia.

[tsmaeder]

Therefore I would tend to unify them by refactoring dialogs to NOT being singletons.

@sdirix I belive that's the wrong approach to fix this "right" as argued above.