Extend multi-window support to Electron (#11642) #49

tsmaeder · 2022-09-14T12:49:48Z

What it does

The main change is to prevent the secondary window from closing until the extracted widget is removed from the window. This includes waiting until any close handling (including dialogs) are finished.

Contributed on behalf of ST Microelectronics.

How to test

Since this just enables the functionality from eclipse-theia#11048 for electron, the "how to test" from that PR applies.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

lucas-koehler

Hi @tsmaeder ,

I tested the changes and, unfortunately, still noticed a few issues.

First, the updated examples/electron/tsconfig.json should be committed with the reference to the secondary-window extension.

Second, I could still get IllegalAccess errors sometimes. Albeit, I feel their frequency has gone down. To test this I used the cat coding extension as a simple widget and the draw.io integration as a custom editor.

On Linux (Ubuntu 20.04), widgets could be reopened even if an IllegalAccess was reported. It seemed that this was more likely with activated autoSave on the custom editor.

On Windows 11 (VM), widgets could sometimes not be re-opened after an IllegalAccess was reported.

On both systems, closing the draw io custom editor via the Open Editors view did close the widget in the external window but the window stayed open as an empty shell.

Besides these issues, I also discovered positive news about the auto save:
With the draw io extension, auto save was applied in the editor. Meaning, when the setting was activated, the editor automatically persisted changes. Furthermore, the save dialog worked when closing a dirty draw io editor in an external window: The closing of the widget and window were stopped until the dialog was closed. The dialog was opened in the main window. However, I think this is another issue: eclipse-theia#11647

tsmaeder · 2022-09-19T12:11:57Z

Hi @lucas-koehler I don't ever get any "illegal access" with the code in this PR on Window 10. Do you have steps to reproduce and frequencies if not repeatable?

lucas-koehler · 2022-09-19T13:20:02Z

Hi @tsmaeder,
I have two sencarios to reproduce this:

Cat Coding

Repeatedly opening and closing the cat coding widget in a secondary window. Frequency was after about 20 tries. I repeated this 2 times.

Custom editor

Enable autosave
opening a draw io diagram, change something, immediately close window
this seems to work quite reliably. However, the Illegal Access seems to have different consequences as the widget can be re-opened.

illegal-access-auto-save.mp4

tsmaeder · 2022-09-19T13:31:40Z

Thx @lucas-koehler. Am I correct that both scenarios are with "autosave" on?

tsmaeder · 2022-09-20T12:32:14Z

I'm having trouble reproducing the problem reliably: it just seems to randomly happen. When I start adding console output to track the problem, the problem goes away. That means that I can't just try out stuff and see if if fixes the problem, since I can't verify a fix if I can't reasonably provoke the problem. <sigh>

paul-marechal · 2022-09-20T23:09:19Z

When testing on Windows I sometime get in a state where the secondary windows opened is empty and I get Uncaught illegal access when I close the Window. I also noticed that the Theia/PhosphorJS widget seems to be lingering from this point onward, preventing the webview from opening again (I tested using the Hex Editor extension).

I cannot find a surefire way to reproduce it but I'm also able to end up in a state where the webview widget is leaked which prevents me from opening the webview, and this without triggering any error. It seems to somehow remove parts of the editor title bar which is odd:

By "widget leaked" I mean that the widget is found in the following piece of logic despite not being on visible in Theia:

custom-editor-opener.tsx#L68

Only when widget is undefined does it seem to show the webview.

tsmaeder · 2022-09-21T07:24:00Z

Unfortunately, it's quite hard to reproduce this issue for me: sometimes I can try for 15 minutes and not reproduce it, sometimes it happens right away. After lengthy experimentation, I've found the following scenario that reproduces the issue quite reliably. This scenario uses the drawio custom editor

create two files x.drawio and bar.drawio
Open x.drawio, then bar.drawio
Move x.drawio to a secondary window
Use the "Open Editors" view in the navigator to bring the secondary window to the front
Move a figure in the x.drawio editor and immediately close the secondary window
Observe the "illegal access" in the main window browser log

tsmaeder · 2022-09-21T07:26:16Z

Here's a recording of the scenario:

2022-09-20.15-44-38.mp4

tsmaeder · 2022-09-21T08:40:23Z

I've added code to remove the 'close' handler from the electron window when it is hit. For me, that seems to fix the problem, but @lucas-koehler can still reproduce the issue. That the electron-remote stuff is involved somehow would make sense, though. A thing to try would be to remove the electron-remote stuff and see if that fixes the problem.

tsmaeder · 2022-09-21T08:41:29Z

I'll go back to working on eclipse-theia#11643 for the time being.

tsmaeder · 2022-09-27T12:36:08Z

I had some further insights on this problem:

Clean handling of saving dirty editors will not be possible (using the save/don't save/cancel dialog) will not be possible in the browser case: we cannot really prevent browser windows from closing: the best we can do is pop up the native dialog asking whether to "leave the page without saving", same as we do when we close the main window. This prevents the approach of doing a proper remove of the widget and postponing the close after close actions have finished.
The approach should work in Electron, though, by adding an on('close') handler.
- Update: registering a 'close' handler with electronRemote does not seem to prevent closing of the browser window, even if we do a "preventDefault". This needs debugging.

tsmaeder · 2022-09-27T13:53:48Z

Confirmed that the close handler works fine when attaching it to the newly create window in the electron main process. So either we're doing something wrong in the current logic or we need to figure out how to call the proper pre-close logic from the electron main process.

tsmaeder · 2022-10-13T07:50:16Z

@lucas-koehler I've updated with my latest changes and rebased on origin/master. Could you have another look?

Fixes eclipse-theia#11642 The main change is to prevent the secondary window from closing until the extracted widget is removed from the window. This includes waiting until any close handling (including dialogs) are finished. Since we cannot reliably prevent closing windows in the browser case, we either save or discard unsaved changes according to the autosave settings Contributed on behalf of ST Microelectronics. Signed-off-by: Thomas Mäder <[email protected]>

lucas-koehler

Browser works as expected given the limitations.
Unfortunately, the IllegalAccess is back using the proven reproduction approach with drawio and two editors.
Idk why that happens as it was not reproducible for me back when I tested the https://github.com/tsmaeder/theia/commits/11642_electron_secondary_window_2 branch a week or so ago.

It was reproducible after one or two tries.
Additional info:
Ubuntu 20.04
Default plugins + drawio editor and cat coding
Node 16.15.1, yarn 1.22.15
I even cloned a new repo with only your branch to avoid some left over pollution of the repo.

lucas-koehler · 2022-10-14T08:56:04Z

packages/core/src/browser/shell/application-shell.ts

-        ]);
+
+        if (ExtractableWidget.is(current) && current.secondaryWindow && !current.isClosing) {
+            current.secondaryWindow!.close();


With the current.secondaryWindow check, the ! shouldn't be necessary.

Suggested change

current.secondaryWindow!.close();

current.secondaryWindow.close();

lucas-koehler · 2022-10-14T09:13:01Z

packages/core/src/browser/widgets/extractable-widget.ts

@@ -22,6 +22,9 @@ import { Widget } from './widget';
 export interface ExtractableWidget extends Widget {
    /** Set to `true` to mark the widget to be extractable. */
    isExtractable: boolean;
+
+    /** State variable to keep track of recursive attempty to close the secondary window */


Suggested change

/** State variable to keep track of recursive attempty to close the secondary window */

/** State variable to keep track of recursive attempts to close the secondary window */

lucas-koehler · 2022-10-14T09:14:00Z

packages/core/src/electron-browser/window/electron-secondary-window-service.ts

        electronRemote.getCurrentWindow().webContents.once('did-create-window', newElectronWindow => {
-            newElectronWindow.setMenuBarVisibility(false);
+            // newElectronWindow.setMenuBarVisibility(false);


Either comment this back in or remove the line completely. I think hiding the menu in secondary windows generally makes sense with the current state (i.e. it's just the default electron menu)

lucas-koehler · 2022-10-14T09:15:55Z

packages/plugin-ext/src/main/browser/webview/webview.ts

@@ -239,11 +240,17 @@ export class WebviewWidget extends BaseWidget implements StatefulWidget, Extract
    }

    protected forceHide(): void {
+        console.log('forcing hide');


The log messages introduced in this file seem like debug logs from development. They should probably be removed again ;)

lucas-koehler · 2022-10-24T13:31:30Z

packages/core/src/browser/shell/application-shell.ts

-        ]);
+
+        if (ExtractableWidget.is(current) && current.secondaryWindow && !current.isClosing) {
+            current.secondaryWindow!.close();


Suggested change

current.secondaryWindow!.close();

current.secondaryWindow.close();

lucas-koehler · 2022-10-24T15:10:39Z

packages/core/src/browser/secondary-window-handler.ts

+                await this.applicationShell.closeWidget(widget.id, closeOptions);
+                widget.isClosing = false;
+                return widget.isDisposed;


await this.applicationShell.closeWidget(widget.id, closeOptions); closes the secondary window that the widget resides in with the changes at application-shell.ts https://github.com/eclipsesource/theia/pull/49/files#diff-cb4b87cc3dc274963d24f6fd5682ad9c89cd51d94726100eb2e3c92421e5c171R1565-R1566

Afterwards, the widget is accessed to set its isClosing flag. Maybe this could cause problems with IllegalAccess?
This is especially intereseting because this was not the case on the last known working state at cdamus@2b3d733

The changes stem from the browser save handling as far as I can see but affect the electron implementation in this way, too. It is called here: https://github.com/eclipsesource/theia/pull/49/files#diff-563e180229ce37170c43089f2a51e72ab90eead76b88b8a4300365d8373f93c3R51

All I can say is that this shouldn't be a problem, since the widget is an object that lives in the context of the main browser window. But it's a diff I should look at.

tsmaeder · 2022-10-31T09:46:53Z

Some more data:

I retrieved the originals "fixed" branch from https://github.com/cdamus/theia/tree/11642_electron_secondary_window_2
I build that branch and was not able to trigger the "illegal access"
I did a rebase on the latest master of that branch locally and was able to reproduce the "illegal access" once, but not as easily as in the code in this PR.

Next, I'll try to reproduce in the branch from this PR in order to make sure we're not seeing some artifact.

tsmaeder · 2022-10-31T09:48:11Z

I went through the compiled PhosphorJS code (widget.js) and noticed that widget.id is actually a computed property that accesses the node. Hmh...that is something to look at.

tsmaeder · 2022-10-31T13:38:02Z

Another note: I can reproduce the "illegal access" as well with terminal windows (though not as reliably), so this does not seem to have anything to do with the particularities of webviews.

tsmaeder · 2022-10-31T16:25:31Z

For what it's worth, I got a case of "illegal access" (with a stack trace) from a section of the electron renderer windows init code, from the section that assigned a window error handler inside the "nodeIntegration=true" branch of execution.

tsmaeder · 2022-11-01T17:13:41Z

So here's what I tried:

Rebased the original tested change to commit before terminal secondary windows: => could not reproduce
Rebased to commit introducing terminal secondary windows => could reproduce.

As always, this could be conincidence: I'll cross-check.

tsmaeder · 2022-11-02T12:40:16Z

So I turned off the keyboard & theme window registrations in a post-terminal commit rebased branch and I can reproduce the problem, but only after numerous attempty.

tsmaeder · 2022-11-02T13:10:59Z

I went back to the state rebased on before the external terminals and I can (after around 20 tries) reproduce the issue. Back to square one.

tsmaeder · 2022-11-02T13:59:24Z

Bad news: I retested the original version that we thought was error-free and was able to provoke the "illegal access" after around 20 tries. The fact seems to be that if we remove bogus accesses (like the one where we keep references to the closed window's document when we update the theme), we can make the errors less likely. This kind of gives me hope that the error is something we cause. On the other hand, we're not really any closer to finding fixing this problem for good.

tsmaeder · 2022-11-03T08:54:36Z

Another candidate for causing the problem is the "open editors" widget: refreshing a tree (by assigning to the root) is always async with no ability to wait for the refresh to end.

tsmaeder · 2022-11-08T14:49:59Z

Another thing to try would be to make the the creation of the secondary window loop around to the main electron thread cleanly instead of using electron-remote.

Signed-off-by: Thomas Mäder <[email protected]>

tsmaeder · 2022-11-08T16:09:02Z

With the latest commit, I haven't been able to reproduce the "illegal access" in my manual testing with the drawio plugin and terminals.

eneufeld · 2022-11-16T14:43:22Z

packages/core/src/electron-main/theia-electron-window.ts

+                            // to prevent "illegal access" errors. Unfortunately, it is unclear what 
+                            // exactly causes the errors.
+                            this._window.focus();
+                            setTimeout(() => {


I suggest to replace lines 101 to 104 with a simple
newWindow.destroy();
When reacting to the newWindow close event, the default propagation of event is stopped in order to check that the second window is safe to be closed. After the check the window was calling close which again triggers the close event, what is not needed. So destroy is the more accurate method to use.
With the change most/all? Illegal access errors are gone.

That was to early, unfortunately the error can still be reproduced after a while.

eneufeld · 2022-11-16T14:49:20Z

packages/core/src/electron-main/theia-electron-window.ts

+                            setTimeout(() => {
+                                closingState = ClosingState.readyToClose;
+                                newWindow.close();
+                            }, 100);


Suggested change

setTimeout(() => {

closingState = ClosingState.readyToClose;

newWindow.close();

}, 100);

closingState = ClosingState.readyToClose;

newWindow.destroy();

eneufeld · 2022-11-21T08:58:54Z

I think there is a memory leak when detaching the windows.
Here is a heaptimeline https://drive.google.com/file/d/1Xp3L0vxjzWvJHY589s-UKBzu7q_4ueAH/view?usp=share_link . Unfortunately I can't directly attach it as the file is to big.

tsmaeder · 2022-12-06T10:57:19Z

Debugging this on the native level reveals that something goes wrong when handling setImmediate() callbacks while closing down the Javascript environment. Not sure exactly what's going on here, since I can't build this old version of electron we're using with full debug symbols (pdb files get > 4Gb, which the old tooling can't handle). Newer versions of electron I've tried don't work with Theia for various reasons or are broken in various ways.
Anyway: here's the stack trace:

electron.exe!v8::internal::`anonymous namespace'::Invoke(v8::internal::Isolate *) Line 320 (c:\Users\thomas\code\testdata\electron\src\v8\src\execution\execution.cc:320)
electron.exe!v8::internal::Execution::Call(v8::internal::Handle<v8::internal::Object>) Line 474 (c:\Users\thomas\code\testdata\electron\src\v8\src\execution\execution.cc:474)
electron.exe!v8::Function::Call(v8::Local<v8::Context>) Line 5090 (c:\Users\thomas\code\testdata\electron\src\v8\src\api\api.cc:5090)
electron.exe!node::InternalMakeCallback(node::Environment *) Line 199 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\src\api\callback.cc:199)
electron.exe!node::MakeCallback(v8::Isolate *) Line 271 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\src\api\callback.cc:271)
electron.exe!node::Environment::CheckImmediate() Line 928 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\src\env.cc:928)
electron.exe!uv_check_invoke(uv_loop_s *) Line 121 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\deps\uv\src\win\loop-watcher.c:121)
electron.exe!uv_run(uv_loop_s *) Line 630 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\deps\uv\src\win\core.c:630)
electron.exe!node::Environment::CleanupHandles() Line 647 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\src\env.cc:647)
electron.exe!node::Environment::RunCleanup() Line 684 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\src\env.cc:684)
electron.exe!node::FreeEnvironment(node::Environment *) Line 375 (c:\Users\thomas\code\testdata\electron\src\third_party\electron_node\src\api\environment.cc:375)
electron.exe!electron::ElectronRendererClient::WillReleaseScriptContext(v8::Local<v8::Context>) Line 161 (c:\Users\thomas\code\testdata\electron\src\electron\shell\renderer\electron_renderer_client.cc:161)
electron.exe!electron::ElectronRenderFrameObserver::WillReleaseScriptContext(v8::Local<v8::Context>) Line 163 (c:\Users\thomas\code\testdata\electron\src\electron\shell\renderer\electron_render_frame_observer.cc:163)
electron.exe!content::RenderFrameImpl::WillReleaseScriptContext(v8::Local<v8::Context>) Line 4460 (c:\Users\thomas\code\testdata\electron\src\content\renderer\render_frame_impl.cc:4460)
electron.exe!blink::LocalFrameClientImpl::WillReleaseScriptContext(v8::Local<v8::Context>) Line 289 (c:\Users\thomas\code\testdata\electron\src\third_party\blink\renderer\core\frame\local_frame_client_impl.cc:289)
electron.exe!blink::LocalWindowProxy::DisposeContext(blink::WindowProxy::Lifecycle) Line 112 (c:\Users\thomas\code\testdata\electron\src\third_party\blink\renderer\bindings\core\v8\local_window_proxy.cc:112)
electron.exe!blink::WindowProxy::ClearForClose() Line 69 (c:\Users\thomas\code\testdata\electron\src\third_party\blink\renderer\bindings\core\v8\window_proxy.cc:69)
electron.exe!blink::WindowProxyManager::ClearForClose() Line 24 (c:\Users\thomas\code\testdata\electron\src\third_party\blink\renderer\bindings\core\v8\window_proxy_manager.cc:24)
electron.exe!blink::Frame::Detach(blink::FrameDetachType) Line 131 (c:\Users\thomas\code\testdata\electron\src\third_party\blink\renderer\core\frame\frame.cc:131)
electron.exe!blink::Page::WillBeDestroyed() Line 972 (c:\Users\thomas\code\testdata\electron\src\third_party\blink\renderer\core\page\page.cc:972)

tsmaeder · 2022-12-06T11:00:07Z

I am not confident that I'll get anywhere without being able to look at local variables. I'm not fit enough with my native debugging to look at the stack in hex, so it's important to get a build at symbol_level = 2. I'll have to try on Linux, I guess.

tsmaeder changed the title ~~Extends multi-window support to Electron (#11642)~~ Extend multi-window support to Electron (#11642) Sep 14, 2022

sdirix requested a review from lucas-koehler September 14, 2022 12:54

lucas-koehler requested changes Sep 19, 2022

View reviewed changes

tsmaeder force-pushed the 11642_electron_secondary_window branch from 3fc6e2b to 06885a4 Compare October 13, 2022 07:47

tsmaeder force-pushed the 11642_electron_secondary_window branch from 06885a4 to ea8db47 Compare October 14, 2022 09:10

tsmaeder requested a review from lucas-koehler October 14, 2022 09:10

lucas-koehler reviewed Oct 14, 2022

View reviewed changes

lucas-koehler reviewed Oct 24, 2022

View reviewed changes

Some changes to prevent "Illegal access"

3ddebe4

Signed-off-by: Thomas Mäder <[email protected]>

eneufeld reviewed Nov 16, 2022

View reviewed changes

	current.secondaryWindow!.close();
	current.secondaryWindow.close();

	/** State variable to keep track of recursive attempty to close the secondary window */
	/** State variable to keep track of recursive attempts to close the secondary window */

Extend multi-window support to Electron (#11642) #49

Are you sure you want to change the base?

Extend multi-window support to Electron (#11642) #49

Conversation

tsmaeder commented Sep 14, 2022

What it does

How to test

Review checklist

Reminder for reviewers

lucas-koehler left a comment

Choose a reason for hiding this comment

tsmaeder commented Sep 19, 2022 • edited Loading

lucas-koehler commented Sep 19, 2022 • edited Loading

Cat Coding

Custom editor

tsmaeder commented Sep 19, 2022

tsmaeder commented Sep 20, 2022 • edited Loading

paul-marechal commented Sep 20, 2022

tsmaeder commented Sep 21, 2022

tsmaeder commented Sep 21, 2022

tsmaeder commented Sep 21, 2022

tsmaeder commented Sep 21, 2022

tsmaeder commented Sep 27, 2022

tsmaeder commented Sep 27, 2022

tsmaeder commented Oct 13, 2022

lucas-koehler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Oct 31, 2022

tsmaeder commented Oct 31, 2022

tsmaeder commented Oct 31, 2022

tsmaeder commented Oct 31, 2022 • edited Loading

tsmaeder commented Nov 1, 2022

tsmaeder commented Nov 2, 2022

tsmaeder commented Nov 2, 2022

tsmaeder commented Nov 2, 2022

tsmaeder commented Nov 3, 2022

tsmaeder commented Nov 8, 2022

tsmaeder commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eneufeld Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

eneufeld commented Nov 21, 2022

tsmaeder commented Dec 6, 2022

tsmaeder commented Dec 6, 2022

tsmaeder commented Sep 19, 2022 •

edited

Loading

lucas-koehler commented Sep 19, 2022 •

edited

Loading

tsmaeder commented Sep 20, 2022 •

edited

Loading

tsmaeder commented Oct 31, 2022 •

edited

Loading

eneufeld Nov 16, 2022 •

edited

Loading