Skip to content
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

Aux window - registration flawed when devtools is opened #209073

Closed
connor4312 opened this issue Mar 28, 2024 · 8 comments
Closed

Aux window - registration flawed when devtools is opened #209073

connor4312 opened this issue Mar 28, 2024 · 8 comments
Assignees
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders verified Verification succeeded workbench-auxwindow Issues related to use of auxiliary ("floating") windows.
Milestone

Comments

@connor4312
Copy link
Member

  1. Set a breakpoint or log here

  2. Move or copy a file into a new floating window

  3. Switch between them a few times. The event fires a couple times but then stops firing

connor4312 added a commit that referenced this issue Mar 28, 2024
Fixes #208895

Should work but testing on aux windows was limited due to #209073
connor4312 added a commit that referenced this issue Mar 29, 2024
Fixes #208895

Should work but testing on aux windows was limited due to #209073
@bpasero
Copy link
Member

bpasero commented Mar 29, 2024

@connor4312 does it reproduce without having devtools opened in any of the windows?

@bpasero bpasero added info-needed Issue requires more information from poster confirmation-pending labels Mar 29, 2024
@bpasero bpasero added this to the April 2024 milestone Mar 29, 2024
@bpasero
Copy link
Member

bpasero commented Mar 29, 2024

@deepak1556 this one takes me by surprise: in the main process I keep track of auxiliary windows and so far I have been using webContents.id as identifier, assuming it would always match BrowserWindow.id. But as it turns out, that is actually not always the case: if I open devtools and restart or reload, I see how suddenly id of WebContents does not match the id of BrowserWindow anymore. In other words, I see an id of 3 for example for the WebContents of the floating window, while its BrowserWindow has an id of 2.

So I do think that I am doing something wrong and I need a different way how to figure out the window ID of a floating window...

I track the creation of floating windows here from the web-contents-created:

app.on('web-contents-created', (event, contents) => {
// Auxiliary Window: delegate to `AuxiliaryWindow` class
if (contents?.opener?.url.startsWith(`${Schemas.vscodeFileResource}:https://${VSCODE_AUTHORITY}/`)) {
this.logService.trace('[aux window] app.on("web-contents-created"): Registering auxiliary window');
this.auxiliaryWindowsMainService?.registerWindow(contents);
}

@connor4312
Copy link
Member Author

connor4312 commented Mar 29, 2024

Ah, that's the repro. Yes, you must have devtools open when you open the aux window. Opening them after the aux window is already open doesn't cause an issue, but once you do open them then the event won't fire properly until vs code is restarted

@deepak1556
Copy link
Contributor

I keep track of auxiliary windows and so far I have been using webContents.id as identifier, assuming it would always match BrowserWindow.id

That is not the intended contract in the runtime, a BrowserWindow can have multiple WebContents at any point and their ids are independent. WebContent ids are incremental within the same process, so it purely depends on the creation order.

I need a different way how to figure out the window ID of a floating window...

You can use BrowserWindow.getWebContents to get the owner window and that should give you the right id.

@bpasero
Copy link
Member

bpasero commented Mar 29, 2024

@deepak1556 I tried that, but at the time web-contents-created fires, BrowserWindow.fromWebContents returns undefined. In other words, the web content is being created before the window is there...

So essentially, I have no way to find the window for the web contents so early.

@bpasero bpasero changed the title layoutService.onDidChangeActiveContainer doesn't fire consistently Aux window - registration flawed when devtools is opened Mar 29, 2024
@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug workbench-auxwindow Issues related to use of auxiliary ("floating") windows. and removed info-needed Issue requires more information from poster confirmation-pending labels Mar 29, 2024
@deepak1556
Copy link
Contributor

I think maintaining a Map<WebContents, AuxWindow> would suit this scenario, you can access the webContents a BrowserWindow owns via window.webContents and this will be valid even at browser-window-created. Should be reliable than id.

@bpasero
Copy link
Member

bpasero commented Mar 29, 2024

Yeah, an alternative would be to pro-long aux-window handshake between main and renderer until the BrowserWindow is there but I think thats overkill. I will see to keep aux windows identified by webContents.id. Its a bit inconsistent because both aux and main window share the same base class and the one will use webContents.id and the other BrowserWindow.id...

@bpasero bpasero closed this as completed in 4ee85ac Apr 8, 2024
@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Apr 8, 2024
@VSCodeTriageBot VSCodeTriageBot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Apr 9, 2024
@bpasero bpasero added authentication Issues with the Authentication platform author-verification-requested Issues potentially verifiable by issue author and removed authentication Issues with the Authentication platform labels Apr 9, 2024
@VSCodeTriageBot
Copy link
Collaborator

This bug has been fixed in the latest release of VS Code Insiders!

@connor4312, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version f8d35f6 of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!

@connor4312 connor4312 added the verified Verification succeeded label Apr 24, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders verified Verification succeeded workbench-auxwindow Issues related to use of auxiliary ("floating") windows.
Projects
None yet
Development

No branches or pull requests

4 participants