[Bug]: Blank page on location change / reload if a filesystem asynchronous call was pending

[remss]

Preflight Checklist

• I have read the Contributing Guidelines for this project.
• I agree to follow the Code of Conduct that this project adheres to.
• I have searched the issue tracker for a bug report that matches the one I want to file, without success.

Electron Version

17.1.1

What operating system are you using?

Windows

Operating System Version

Windows 10 21H2

What arch are you using?

x64

Last Known Working Electron version

13.6.9

Expected Behavior

Assuming a BrowserWindow like this:

const mainWindow = new BrowserWindow({
    ...
    webPreferences: {
        nodeIntegration: true,
        contextIsolation: false,
    }
})

And an asynchronous filesystem call on fs.promises API

// renderer.js
const fs = require('fs');
async function asyncStuff() {
    let output = await fs.promises.readFile("datafile");
}

Then trying to reload the page when the fs asynchronous call is not finished yet will make the page fail (blank page).

// renderer.js
async function testCrash() {
    asyncStuff() // No await here
    window.location.reload() // Here the renderer will crash
})

I expect that the page reloads correctly.

Actual Behavior

The page is blank, and DevTools are disconnected.

Testcase Gist URL

No response

Additional Information

Notes:

• Replace the fs.promises.readFile() by fs.readFileSync() and the page will reload fine
• Replace the fs.promises.readFile() by fs.readFile() with callback and the page will reload fine
• The bug is here also with a window.location.href
• I tried with other async api and the page works fine (like https.get(), timers.promises.setTimeout(), or fetch())

I created a repo to reproduce the bug: https://github.com/remss/electron-issue-reload
Just run npm i && npm start then click on reloadMeWithAsyncStuff button to see the blank page.

[DanielMcAssey]

Getting this too

[codebytere]

Gist version: https://gist.github.com/60969896fee8b6e74450e33309e7974b

Related CLs:

• https://chromium-review.googlesource.com/c/v8/v8/+/3293410
• https://chromium-review.googlesource.com/c/v8/v8/+/3386600

We're aware of this, been trying to repro it for a while so thanks! cc @deepak1556

[DanielMcAssey]

Just to add I cant replicate this on 16.1.0

[codebytere]

@DanielMcAssey correct, it's from a version of V8 introduced in 17.

[deepak1556]

@jkleinsc the crash looks unrelated to cppgc and #33252 does not fix this one, reopening for further investigation.

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0xffffffffffffffff
Process uptime: 3 seconds

Thread 0 (crashed)
 0  electron.exe!std::__1::__hash_table<std::__1::__hash_value_type<node::FastStringKey,node::BaseObjectPtrImpl<node::BaseObject,0> >,std::__1::__unordered_map_hasher<node::FastStringKey,std::__1::__hash_value_type<node::FastStringKey,node::BaseObjectPtrImpl<node::BaseObject,0> >,node::FastStringKey::Hash,std::__1::equal_to<node::FastStringKey>,1>,std::__1::__unordered_map_equal<node::FastStringKey,std::__1::__hash_value_type<node::FastStringKey,node::BaseObjectPtrImpl<node::BaseObject,0> >,std::__1::equal_to<node::FastStringKey>,node::FastStringKey::Hash,1>,std::__1::allocator<std::__1::__hash_value_type<node::FastStringKey,node::BaseObjectPtrImpl<node::BaseObject,0> > > >::find<node::FastStringKey>(node::FastStringKey const &) [__hash_table : 2394 + 0x0]
    rax = 0xcdcdcdcdcdcdcdcd   rdx = 0x2555555555555555
    rcx = 0x000012fe00a8c9b0   rbx = 0x0101010101010101
    rsi = 0x000000faad7fc200   rdi = 0x0000000000000028
    rbp = 0x0000000000000000   rsp = 0x000000faad7fba50
     r8 = 0x00007ff7a5081220    r9 = 0x7b60c3798090b241
    r10 = 0x000000faad7fbaa8   r11 = 0x7b60c3798090b241
    r12 = 0xffffffffffffffff   r13 = 0x000012fe01284000
    r14 = 0x0000000000000006   r15 = 0xcdcdcdcdcdcdcdcd
    rip = 0x00007ff7a0bd5c3c
    Found by: given as instruction pointer in context
 1  electron.exe!node::fs::GetReqWrap(v8::FunctionCallbackInfo<v8::Value> const &,int,bool) [node_file-inl.h : 230 + 0x3f]
    rbx = 0x0101010101010101   rbp = 0x0000000000000000
    rsp = 0x000000faad7fba80   r12 = 0xffffffffffffffff
    r13 = 0x000012fe01284000   r14 = 0x0000000000000006
    r15 = 0xcdcdcdcdcdcdcdcd   rip = 0x00007ff7a0bfb592
    Found by: call frame info
 2  electron.exe!static void node::fs::Read(const class v8::FunctionCallbackInfo<v8::Value> & const) [node_file.cc : 2067 + 0x10]
    rbx = 0x0101010101010101   rbp = 0x0000000000000000
    rsp = 0x000000faad7fbae0   r12 = 0xffffffffffffffff
    r13 = 0x000012fe01284000   r14 = 0x0000000000000006
    r15 = 0xcdcdcdcdcdcdcdcd   rip = 0x00007ff7a0bfece8
    Found by: call frame info
 3  electron.exe!v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [api-arguments-inl.h : 152 + 0x9]
    rbx = 0x0101010101010101   rbp = 0x0000000000000000
    rsp = 0x000000faad7fbde0   r12 = 0xffffffffffffffff
    r13 = 0x000012fe01284000   r14 = 0x0000000000000006
    r15 = 0xcdcdcdcdcdcdcdcd   rip = 0x00007ff79cbdc6b5
    Found by: call frame info
 4  electron.exe!static class v8::internal::MaybeHandle<v8::internal::Object> v8::internal::`anonymous namespace'::HandleApiCallHelper<0>(class v8::internal::Isolate *, class v8::internal::Handle<v8::internal::HeapObject>, class v8::internal::Handle<v8::internal::HeapObject>, class v8::internal::Handle<v8::internal::FunctionTemplateInfo>, class v8::internal::Handle<v8::internal::Object>, class v8::internal::BuiltinArguments) [builtins-api.cc : 112 + 0xb]
    rbx = 0x0101010101010101   rbp = 0x0000000000000000
    rsp = 0x000000faad7fbee0   r12 = 0xffffffffffffffff
    r13 = 0x000012fe01284000   r14 = 0x0000000000000006
    r15 = 0xcdcdcdcdcdcdcdcd   rip = 0x00007ff79cbda7a7
    Found by: call frame info
 5  electron.exe!static class v8::internal::Object v8::internal::Builtin_Impl_HandleApiCall(class v8::internal::BuiltinArguments, class v8::internal::Isolate *) [builtins-api.cc : 142 + 0x3a]
    rbx = 0x0101010101010101   rbp = 0x0000000000000000
    rsp = 0x000000faad7fc000   r12 = 0xffffffffffffffff
    r13 = 0x000012fe01284000   r14 = 0x0000000000000006
    r15 = 0xcdcdcdcdcdcdcdcd   rip = 0x00007ff79cbd8859
    Found by: call frame info
 6  electron.exe!v8::internal::Builtin_HandleApiCall(int,unsigned __int64 *,v8::internal::Isolate *) [builtins-api.cc : 130 + 0x2d]
    rbx = 0x0101010101010101   rbp = 0x0000000000000000
    rsp = 0x000000faad7fc0f0   r12 = 0xffffffffffffffff
    r13 = 0x000012fe01284000   r14 = 0x0000000000000006
    r15 = 0xcdcdcdcdcdcdcdcd   rip = 0x00007ff79cbd81be
    Found by: call frame info
 7  0x45cb07ededfc
    rbx = 0x0101010101010101   rbp = 0x0000000000000000
    rsp = 0x000000faad7fc160   r12 = 0xffffffffffffffff
    r13 = 0x000012fe01284000   r14 = 0x0000000000000006
    r15 = 0xcdcdcdcdcdcdcdcd   rip = 0x000045cb07ededfc
    Found by: call frame info

[codebytere]

@deepak1556 sounds good, i'll dig into it.

[codebytere]

@DanielMcAssey I can repro it starting with 16.0.5 using my above gist.

Looks like it was caused by 73ba0cb cc @zcbenz

[remss]

Using my repro case, here the result with different versions of Electron:

• v13.6.9 : PASS, I cannot reproduce the issue
• v14.2.7 : FAIL, I reproduce the issue
• v15.4.1 : FAIL, I reproduce the issue
• v16.1.0 : FAIL, I reproduce the issue
• v17.1.1 : FAIL, I reproduce the issue

[deepak1556]

@codebytere to further narrow down the trigger, most of the changes in that PR are guarded behind window opener checks, I think the explicit call to run microtasks

electron/shell/renderer/electron_renderer_client.cc

Line 160 in 4bdb50e

gin_helper::MicrotasksScope microtasks_scope(env->isolate());

before destroying the node environment might be the most likely culprit here since the UAF is from the fs promises api.

[deepak1556]

node::FreeEnvironment disabled JS execution https://github.com/nodejs/node/blob/0367b5c35ea0f98b323175a4aaa8e651af7a91e7/src/api/environment.cc#L368-L369 , so not sure if we want to run the microtasks at this point.

[codebytere]

@deepak1556 yep sorry should have updated this - that's also what I found and have been working to fix! Also not sure if we want to run microtasks, or rather force flush the pending tasks. If we don't run them however it DCHECKs:

#
# Fatal error in ../../v8/src/api/api-inl.h, line 185
# Debug check failed: microtask_queue->GetMicrotasksScopeDepth() || !microtask_queue->DebugMicrotasksScopeDepthIsZero().
#
#
#
#FailureMessage Object: 0x7ff7bbf64fe0
0   Electron Framework                  0x000000012a4645b9 base::debug::CollectStackTrace(void**, unsigned long) + 9
1   Electron Framework                  0x000000012a387313 base::debug::StackTrace::StackTrace() + 19
2   Electron Framework                  0x000000012c2eafbd gin::(anonymous namespace)::PrintStackTrace() + 45
3   Electron Framework                  0x000000012bd38296 V8_Fatal(char const*, int, char const*, ...) + 326
4   Electron Framework                  0x000000012bd37d35 v8::base::(anonymous namespace)::DefaultDcheckHandler(char const*, int, char const*) + 21
5   Electron Framework                  0x00000001275acf90 v8::CallDepthScope<true>::~CallDepthScope() + 1040
6   Electron Framework                  0x000000012757ed26 v8::Promise::Resolver::Resolve(v8::Local<v8::Context>, v8::Local<v8::Value>) + 566
7   Electron Framework                  0x000000012f4c0a3c node::fs::FSReqPromise<node::AliasedBufferBase<double, v8::Float64Array, void> >::Resolve(v8::Local<v8::Value>) + 220
8   Electron Framework                  0x000000012f4cc84b node::fs::AfterInteger(uv_fs_s*) + 187

reverting to that would fix the crash, but i'd prefer a more proper fix 🤔

[deepak1556]

During the deletion of script context, we make sure to stop running node callbacks

electron/shell/common/node_bindings.cc

Lines 606 to 610 in 4bdb50e

	// When doing navigation without restarting renderer process, it may happen
	// that the node environment is destroyed but the message loop is still there.
	// In this case we should not run uv loop.
	if (!env)
	return;

if the main frame is getting disposed

electron/shell/renderer/electron_renderer_client.cc

Lines 150 to 152 in 4bdb50e

	// The main frame may be replaced.
	if (env == node_bindings_->uv_env())
	node_bindings_->set_uv_env(nullptr);

, I think the above promise tasks are still run during environment deletion probably because of this call https://github.com/nodejs/node/blob/a01302b8dfaa9aa6290cd6bee552c4ce30dca34c/src/api/environment.cc#L384-L386. Can you check if the DCHECK triggers when the drain tasks call is commented out ? I am wondering if the drain tasks should be called before RunCleanup which basically will delete all open handles and api wrappers from node.

[remss]

@codebytere The bug persists using electron@18.0.0-beta.4
I updated my repro case https://github.com/remss/electron-issue-reload/tree/electron-reload-issue

[deepak1556]

@remss the fix for this issue has not been merged into the 18-x-y branch yet, you can track it at #33302

[remss]

Ho sorry, I was confused by the release note of https://github.com/electron/electron/releases/tag/v18.0.0-beta.4 and random crash in renderer.

[liangwenzhong]

any solution ?

[DanielMcAssey]

This was fixed a minor version or 2 ago