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

'webview.getWebContents()' is deprecated and will be removed #95955

Closed
bpasero opened this issue Apr 23, 2020 · 8 comments · Fixed by #100342
Closed

'webview.getWebContents()' is deprecated and will be removed #95955

bpasero opened this issue Apr 23, 2020 · 8 comments · Fixed by #100342
Assignees
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders webview Webview issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Apr 23, 2020

Please use 'remote.webContents.fromId(webview.getWebContentsId())' instead

image

Seeing this in my E8 exploration build console. I am not sure we want to support the usage of the remote module. Maybe we can do this access alternatively not requiring it at all?

//cc @deepak1556

@bpasero bpasero added debt Code quality issues webview Webview issues electron-8-update labels Apr 23, 2020
@bpasero
Copy link
Member Author

bpasero commented Apr 23, 2020

Looks like https://www.electronjs.org/docs/breaking-changes#deprecated-webviewgetwebcontents outlines a solution without remote module.

@mjbvz mjbvz added this to the May 2020 milestone Apr 23, 2020
@mjbvz
Copy link
Collaborator

mjbvz commented May 5, 2020

@bpasero @deepak1556 We use the web contents in a few different ways:

Moving all of these to use ipc will be difficult. There may be some other electron APIs that can help us here though

This doesn't block updating electron 8 though, correct?

@deepak1556
Copy link
Collaborator

deepak1556 commented May 5, 2020

This doesn't block electron 8 but will block electron 9 exploration.

Moving all of these to use ipc will be difficult

Can you expand on this ?

Whatever method is being used on webview.getWebContents(), all of them are internally forwarded to the main process over ipc by electron, so any usage we have now can be moved without relying on webContents directly in the renderer.

Also caching the webContents on the renderer side is not a good idea as there is no clear signal for the destruction path, which can lead to issues like #96207 . Handling the webContents method on the main process will avoid such issues.

@bpasero
Copy link
Member Author

bpasero commented May 6, 2020

@deepak1556 does it make sense to push this any further given we already know that webcontents will not work with context isolation? How would the solution look like for iframe?

Can you maybe clarify what the exact conditions are, because I thought about enabling preload+contextIsolation this month, but maybe we cannot do that until we are off webview?

@deepak1556
Copy link
Collaborator

@bpasero we need a bit of planning here on how to tackle this over the next few iterations.

  1. webview.getWebContents() is removed from Electron 9, so current webview usage cannot work after with it moving forward.

  2. when using webview under contextIsolation, electron will throw a deprecation log in the devtools console

Security Warning: A WebContents was just created with both webviewTag and contextIsolation enabled. This combination is fundamentally less secure and effectively bypasses the protections of contextIsolation. We strongly recommend you move away from webviews to OOPIF or BrowserView in order for your app to be more secure

But the usage has not been blocked in electron master yet and it will most likely happen with electron v12 which gives us time to transition to OOPIF based implementation.

Given the above we can start the work for preload+contextIsolation in this iteration and in parallel start exploring the transition to OOPIF which will atleast take one or two iterations.

In this iteration, I am exploring #96307 which is one of the blocker for iframe transition. And the actual work to transition can happen in June or July depending on @mjbvz availability.

And to answer the main question of fixing the usage of webview.getWebContents, we should do it if we want to adopt electron 9 and higher, but if we can wait a couple more iterations for the iframe transition to finish then this work can be avoided.

Just to reiterate the transition to OOPIF will not block adopting preload+contextIsolation work, so please go ahead with it :)

mjbvz added a commit to mjbvz/vscode that referenced this issue May 19, 2020
Part of microsoft#95955

Switches from calling `setIgnoreMenuShortcuts` on the renderer to calling it on the main thread
mjbvz added a commit that referenced this issue May 27, 2020
Part of #95955

Switches from calling `setIgnoreMenuShortcuts` on the renderer to calling it on the main thread
mjbvz added a commit that referenced this issue May 27, 2020
Part of #95955

Switches from calling `setIgnoreMenuShortcuts` on the renderer to calling it on the main thread
mjbvz added a commit that referenced this issue May 27, 2020
* Move webview's use of setIgnoreMenuShortcuts to main processes

Part of #95955

Switches from calling `setIgnoreMenuShortcuts` on the renderer to calling it on the main thread

* Use channels for communication instead of ipc

* Cleanup ipc implementation

* Rename webviewMainService -> WebviewManagerService
@mjbvz mjbvz modified the milestones: May 2020, June 2020 May 29, 2020
@mjbvz
Copy link
Collaborator

mjbvz commented Jun 5, 2020

I quickly explored two other approaches for serving up the content for the webview (the stuff that we control). These would (ideally) let us move away from custom protocols but I couldn't get either working :

  • Use protocol.interceptHttpProtocol to intercept requests to vscode-webview-test.com

    I couldn't get this working for https requests. There also does not appear to be a way to limit which requests are intercepted. Also, I always ended up either crashing VS Code or getting into an intercept loop when I tried using these

  • Use webRequest.onBeforeRequest

    This doesn't seem to provide enough flexibly for serving up custom content when a request is made

mjbvz added a commit that referenced this issue Jun 15, 2020
Fixes #89038
For #95955

This moves the webview local resource protocol back to the main process. It should now also handle remote cases by sending the remote data back to the main process
mjbvz added a commit to mjbvz/vscode that referenced this issue Jun 17, 2020
Fixes microsoft#95955

Our port mapping impl for webview currently relies on `getWebContents` to handle port mapping in the renderer process. This API has been deprecated by electron.

This change instead moves the webview port mapping handler to the main process. To make this change, I also realized that the tunnel service needed to be moved from `vs/workbench` to `vs/platform` so that we could consume it from the main process

Other changes made as part of this refactoring:

-  Register all webview in a `webview` partition. This ensures that our port mapping manger only intercepts requests made inside webviews.

- Send the `webContentsId` to the main process. This is required to implement `onBeforeRequest`. Unfortunatly the referrer always seems to be undefined for these requests

- Have the tunnel service take a resolved authority instead of taking the raw authority. This was required due to the tunnel service moving to `vs/platform`
mjbvz added a commit to mjbvz/vscode that referenced this issue Jun 17, 2020
Fixes microsoft#95955

Our port mapping impl for webview currently relies on `getWebContents` to handle port mapping in the renderer process. This API has been deprecated by electron.

This change instead moves the webview port mapping handler to the main process. To make this change, I also realized that the tunnel service needed to be moved from `vs/workbench` to `vs/platform` so that we could consume it from the main process

Other changes made as part of this refactoring:

-  Register all webview in a `webview` partition. This ensures that our port mapping manger only intercepts requests made inside webviews.

- Send the `webContentsId` to the main process. This is required to implement `onBeforeRequest`. Unfortunatly the referrer always seems to be undefined for these requests

- Have the tunnel service take a resolved authority instead of taking the raw authority. This was required due to the tunnel service moving to `vs/platform`
mjbvz added a commit that referenced this issue Jun 17, 2020
* Move the webview port mapping from renderer to main process

Fixes #95955

Our port mapping impl for webview currently relies on `getWebContents` to handle port mapping in the renderer process. This API has been deprecated by electron.

This change instead moves the webview port mapping handler to the main process. To make this change, I also realized that the tunnel service needed to be moved from `vs/workbench` to `vs/platform` so that we could consume it from the main process

Other changes made as part of this refactoring:

-  Register all webview in a `webview` partition. This ensures that our port mapping manger only intercepts requests made inside webviews.

- Send the `webContentsId` to the main process. This is required to implement `onBeforeRequest`. Unfortunatly the referrer always seems to be undefined for these requests

- Have the tunnel service take a resolved authority instead of taking the raw authority. This was required due to the tunnel service moving to `vs/platform`

* Cleanup and adding url filter
@bpasero
Copy link
Member Author

bpasero commented Jun 28, 2020

@deepak1556 it looks like this was the last dependency on remote module, so I think we can now configure enableRemoteModule: false for all our windows, would you agree?

@deepak1556
Copy link
Collaborator

@bpasero yes that would be safe to add now.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues insiders-released Patch has been released in VS Code Insiders webview Webview issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@bpasero @deepak1556 @mjbvz and others