-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Move the webview port mapping from renderer to main process #100342
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.
I only looked at main.ts and workbench.main
@@ -468,10 +470,12 @@ export class NativeWindow extends Disposable { | |||
// Register external URI resolver | |||
this.openerService.registerExternalUriResolver({ | |||
resolveExternalUri: async (uri: URI, options?: OpenOptions) => { | |||
console.log('xxxxxxx'); |
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.
This should be removed.
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 focused my review on tunnel service and remote explorer related files. Thanks for making sure I saw the changes!
@@ -15,6 +15,10 @@ import { REMOTE_HOST_SCHEME } from 'vs/platform/remote/common/remoteHosts'; | |||
import { IRequestService } from 'vs/platform/request/common/request'; | |||
import { getWebviewContentMimeType } from 'vs/platform/webview/common/mimeTypes'; | |||
|
|||
|
|||
export const webviewPartitionId = 'webview'; |
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.
Is this a temporary partition on purpose, because is only to intercept requests in a isolated way and not for persisting data to disk ?
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.
Yes we use partition to make sure that our onBeforeRequest
handler only intercepts requests from webviews instead of being fired for all requests
I've also added a filter so that we only intercept localhost requests from webviews
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.
Just to note, if the partition is not persistent that is, if it doesn't start with persist:webview
, then the storage for webviews will be temporary, cookies, web storage etc, network auth cache etc or any user data related to the browsing context will not be persisted to disk. Is that the intention here ?
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.
yes, right now webviews are supposed to be entirely ephemeral
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.
got it, thanks for confirming!
return callback(redirect ? { redirectURL: redirect } : {}); | ||
}); | ||
|
||
this._register(toDisposable(() => protocol.unregisterProtocol(Schemas.vscodeWebviewResource))); |
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.
should use sess.protocol
otherwise the unregistration happens on the default session.
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.
Good catch :)
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`
2597b45
to
8c4eaa1
Compare
Fixes #95955
Our port mapping implementation for webviews currently relies on
getWebContents
to handle port mapping in the renderer process. This API has been deprecated by electron. See #95955 for detailsThis change instead moves the webview port mapping handler to the main process. To support this, I also realized that the tunnel service needed to be moved from
vs/workbench
tovs/platform
so that we could consume it from the main processOther 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 implementonBeforeRequest
. Unfortunatly the referrer always seems to be undefined for these requestsHave 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