microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
164.38k stars 29.32k forks source link

Aux window - registration flawed when devtools is opened #209073

Closed connor4312 closed 7 months ago

connor4312 commented 7 months ago
  1. Set a breakpoint or log here https://github.com/microsoft/vscode/blob/e49b522e12d1a55dfe2950af355767c9d5b824aa/src/vs/workbench/contrib/debug/browser/debugToolBar.ts#L195

  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

bpasero commented 7 months ago

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

bpasero commented 7 months ago

@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:

https://github.com/microsoft/vscode/blob/e2dc70e39363281b6894b8ab11dfb6a9b452b56f/src/vs/code/electron-main/app.ts#L393-L400

connor4312 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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.

deepak1556 commented 7 months ago

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 commented 7 months ago

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...

vscodenpa commented 7 months ago

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 f8d35f6a712c59f96078161799435d0e83461556 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!