-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend multi-window support to Electron (#11642) #49
base: master
Are you sure you want to change the base?
Extend multi-window support to Electron (#11642) #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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? |
Hi @tsmaeder, Cat Coding
Custom editor
illegal-access-auto-save.mp4 |
Thx @lucas-koehler. Am I correct that both scenarios are with "autosave" on? |
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> |
When testing on Windows I sometime get in a state where the secondary windows opened is empty and I get 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: Only when |
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
|
Here's a recording of the scenario: 2022-09-20.15-44-38.mp4 |
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. |
I'll go back to working on eclipse-theia#11643 for the time being. |
I had some further insights on this problem:
|
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. |
3fc6e2b
to
06885a4
Compare
@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]>
06885a4
to
ea8db47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
]); | ||
|
||
if (ExtractableWidget.is(current) && current.secondaryWindow && !current.isClosing) { | ||
current.secondaryWindow!.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current.secondaryWindow
check, the !
shouldn't be necessary.
current.secondaryWindow!.close(); | |
current.secondaryWindow.close(); |
@@ -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 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** 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 */ |
electronRemote.getCurrentWindow().webContents.once('did-create-window', newElectronWindow => { | ||
newElectronWindow.setMenuBarVisibility(false); | ||
// newElectronWindow.setMenuBarVisibility(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -239,11 +240,17 @@ export class WebviewWidget extends BaseWidget implements StatefulWidget, Extract | |||
} | |||
|
|||
protected forceHide(): void { | |||
console.log('forcing hide'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The log messages introduced in this file seem like debug logs from development. They should probably be removed again ;)
]); | ||
|
||
if (ExtractableWidget.is(current) && current.secondaryWindow && !current.isClosing) { | ||
current.secondaryWindow!.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current.secondaryWindow!.close(); | |
current.secondaryWindow.close(); |
await this.applicationShell.closeWidget(widget.id, closeOptions); | ||
widget.isClosing = false; | ||
return widget.isDisposed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Some more data:
Next, I'll try to reproduce in the branch from this PR in order to make sure we're not seeing some artifact. |
I went through the compiled PhosphorJS code ( |
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. |
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. |
So here's what I tried:
As always, this could be conincidence: I'll cross-check. |
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. |
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. |
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. |
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. |
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]>
With the latest commit, I haven't been able to reproduce the "illegal access" in my manual testing with the drawio plugin and terminals. |
// to prevent "illegal access" errors. Unfortunately, it is unclear what | ||
// exactly causes the errors. | ||
this._window.focus(); | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was to early, unfortunately the error can still be reproduced after a while.
setTimeout(() => { | ||
closingState = ClosingState.readyToClose; | ||
newWindow.close(); | ||
}, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setTimeout(() => { | |
closingState = ClosingState.readyToClose; | |
newWindow.close(); | |
}, 100); | |
closingState = ClosingState.readyToClose; | |
newWindow.destroy(); |
I think there is a memory leak when detaching the windows. |
Debugging this on the native level reveals that something goes wrong when handling
|
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 |
What it does
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.
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
Reminder for reviewers