@nornagon hence the draft state 😄 , hopefully I can get a minimal test suite from the vscode team.

I spent a few days trying to get a minimal reproduce on vanilla Electron but I found the test is simply about removing the webview shortly after the webview is created (sequence 1, 2, 3, 5, 4 per #24344 (comment)). On a relatively powerful machine, that's nanoseconds.

For example, I wrote a test like below and I can only crash the process 1-2 out of 100 times on some bad days (I'm guessing it's when my machine is on heavy loads)

const v8Util = process.electronBinding('v8_util');
const ipcRendererInternal = v8Util.getHiddenValue(global, 'ipc-internal');
const { ipc } = process.electronBinding('ipc');
function timeoutAsync(n) {
    return new Promise(resolve => {
        setTimeout(() => {
            resolve();
        }, n);
    });
}
const loadWebView = async (id, container, webview, attributes = {}) => {
    for (const [name, value] of Object.entries(attributes)) {
        webview.setAttribute(name, value);
    }
    container.appendChild(webview);
    setTimeout(() => {
        ipcRendererInternal.sendSync('ELECTRON_GUEST_VIEW_MANAGER_DETACH_GUEST', id + 1);
    }, 2);
};
async function test() {
    for (let i = 0; i < 500; i++) {
        let webview = new WebView();
        const container = document.createElement('div');
        document.body.appendChild(container);
        loadWebView(i + 1, container, webview, {
            src: `https://s3.amazonaws.com/duhaime/blog/visualizations/isolation-forests.html`
        });
        await timeoutAsync(10);
    }
}
window.onload = function () {
    test()
}

Even on our CI https://dev.azure.com/vscode/VSCode/_build/results?buildId=48185&view=logs&j=a5e52b91-c83f-5429-4a68-c246fc63a4f7&t=855ed636-0983-5316-9b28-7efa8f10a65b, on which we are seeing most of the crashes, we only saw crashes 1 out of 5 to 10 times. It made me think that if we are going to have a test, which can crash the process 100% of the time, we need to either

• mimic the CI environment and make the CI machine under as heavy load as VS Code integration tests.
• or, have code in electron/chromium directly to slow down the execution, to trigger the racing condition.

but none of above sounds valuable. I'm wondering if there is any other approach that we can take to move this forward?

I'm good without a test in this case.

I'm a little uneasy that this fixes a UAF by shuffling some JavaScript around. It should be impossible to cause a UAF from JS. Can we address this on the C++ side?

It should be impossible to cause a UAF from JS.

For webview it becomes possible because its backing webContents lifetime is tied to either the embedder on the main process or the DOM node on the renderer unlike the other JS objects we manage. Hence this mostly boils down to the mapping we maintain in guest-view-manager, there is a case that is not updating the mapping correctly during destruction of the embedder which would the root cause here. But our destruction sequence is quite complex to mess with, hence this turned out to be the simplest solution for backports. I agree that this doesn't fix the root cause but eliminates the abnormalities around webview creation.

Can we address this on the C++ side?

any ideas ?

@deepak1556 can you please rebase on top of latest master?

We now have ASan tests enabled in master (on Linux only). That might make it easier to create a reliably-failing test.

I am not seeing those crashes anymore.

@zcbenz, @codebytere can you please review?

Failing tests are unrelated, merging.

Release Notes Persisted

Fixed crash in webview creation casued by UAF in the browser process