[BUG][Electron] Page is not a BrowserWindow

[samuelmaddock]

Context:

## System:
 - OS: Windows 10 10.0.19042
 - Memory: 16.79 GB / 31.91 GB
## Binaries:
 - Node: 14.16.1 - C:\Program Files\nodejs\node.EXE
 - Yarn: 1.19.1 - D:\Program Files (x86)\Yarn\bin\yarn.CMD
 - npm: 7.11.2 - C:\Program Files\nodejs\npm.CMD
## Languages:
 - Bash: 4.4.23 - C:\Program Files\Git\usr\bin\bash.EXE
## npmPackages:
 - playwright: ^1.11.1 => 1.11.1

Code Snippet

Creates two Pages: one for the BrowserWindow's WebContents and one for the BrowserView's WebContents.

const { _electron: electron } = require('playwright');

(async () => {
  const app = await electron.launch();

  app.on('window', async page => {
    console.log('new window page', page.url())
  });

  await app.evaluate((electron) => {
    const win = electron.BrowserWindow.getAllWindows()[0]
    const view = new electron.BrowserView()
    view.setBounds({ x: 0, y: 0, width: 300, height: 300 })
    view.webContents.loadURL('https://www.electronjs.org/docs/api/browser-view')
    win.addBrowserView(view)
  });
})();

Describe the bug

Playwright is conflating Electron's BrowserWindow abstraction as an equivalent to Pages. However, it's possible for Electron to have multiple WebContents per BrowserWindow.

In the method below, a new Page will increment the BrowserWindow counter.

playwright/src/server/electron/electron.ts

Lines 73 to 77 in 3aa1471

	private _onPage(page: Page) {
	// Needs to be sync.
	const windowId = ++this._lastWindowId;
	(page as any)._browserWindowId = windowId;
	}

playwright/src/server/electron/electron.ts

Lines 92 to 95 in 3aa1471

	async browserWindow(page: Page): Promise<js.JSHandle<BrowserWindow>> {
	const electronHandle = await this._nodeElectronHandlePromise;
	return await electronHandle.evaluateHandle(({ BrowserWindow }, windowId) => BrowserWindow.fromId(windowId), (page as any)._browserWindowId);
	}

This ID counter will become out of sync when the code snippet above is run. I also suspect this ID becoming out of sync is leading to the bug described in #6748.

---

I believe that Playwright might want to redesign how it stores and retrieves BrowserWindow objects from Electron. Based on the info received from Chrome DevTools Protocol, I suspect the TargetID might be the most stable identifier which could be used.

With some contributions upstream, I could see the TargetID being used to get the associated WebContents, which could then be used to get the owning BrowserWindow.

I'd be open to helping make this happen if it makes sense. Possible new APIs could be defined as followed:

interface WebContents {
  static fromDevToolsTargetId(targetId: string): WebContents;
}

Then getting the BrowserWindow could be done by calling BrowserWindow.fromWebContents(webContents).

// playwright/src/server/electron/electron.ts
async browserWindow(page: Page): Promise<js.JSHandle<BrowserWindow>> {
  const targetId = (page as any)._delegate._targetId;
  const electronHandle = await this._nodeElectronHandlePromise;
  return await electronHandle.evaluateHandle(({ BrowserWindow, webContents }, targetId) => {
    const wc = webContents.fromDevToolsTargetId(targetId)
    return BrowserWindow.fromWebContents(wc)
  }, targetId);
}

[yury-s]

You are right, this is caused by the same issue as #6748 and the problem is indeed in the way we assign ids to the Pages that represent BrowserWindows. You are very welcome to contribute patches that fix this.

[samuelmaddock]

@yury-s Great! I've introduce a PR upstream in Electron which will allow Playwright to use the proposed WebContents lookup I described in this PR's description.

[ggreco]

@yury-s Great! I've introduce a PR upstream in Electron which will allow Playwright to use the proposed WebContents lookup I described in this PR's description.

A few months have passed since your message, is the PR on electron available in stable electron? (12.x)

Given the availability of that patch in electron, what kind of work is needed on playwright to fix the association between page <-> browserwindow?