fix: backport Node nested microtask fix (#20303)

patches/node/.patches

@@ -39,3 +39,4 @@ chore_split_createenvironment_into_createenvironment_and.patch
 chore_handle_default_configuration_not_being_set_in_the_electron_env.patch
 revert_crypto_add_outputlength_option_to_crypto_createhash.patch
 add_openssl_is_boringssl_guard_to_oaep_hash_check.patch
+fix_microtasks.patch

patches/node/fix_microtasks.patch

@@ -0,0 +1,58 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Jeremy Apthorp <nornagon@nornagon.net>
+Date: Mon, 16 Sep 2019 17:50:28 -0400
+Subject: fix microtasks
+
+backports https://github.com/nodejs/node/pull/29581 and https://github.com/nodejs/node/pull/29434
+
+diff --git a/src/api/callback.cc b/src/api/callback.cc
+index 43ccfafd9f2c85e23a9ea6277e88e4864e287905..3c518870c9c8d92f3dfcd6c270f5e023e3b69633 100644
+--- a/src/api/callback.cc
++++ b/src/api/callback.cc
+@@ -12,6 +12,7 @@ using v8::HandleScope;
+ using v8::Isolate;
+ using v8::Local;
+ using v8::MaybeLocal;
++using v8::MicrotasksScope;
+ using v8::NewStringType;
+ using v8::Object;
+ using v8::String;
+@@ -100,7 +101,7 @@ void InternalCallbackScope::Close() {
+ 
+   if (!env_->can_call_into_js()) return;
+   if (!tick_info->has_tick_scheduled()) {
+-    env_->isolate()->RunMicrotasks();
++    MicrotasksScope::PerformCheckpoint(env_->isolate());
+   }
+ 
+ #if 0  // FIXME(codebytere): figure out why this check fails/causes crash
+diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc
+index e6b4d0b8e211cdb1fef4759457c2550e28448360..918796ba77d80cf66324164a930f8068e0622ccb 100644
+--- a/src/node_task_queue.cc
++++ b/src/node_task_queue.cc
+@@ -21,6 +21,7 @@ using v8::kPromiseRejectWithNoHandler;
+ using v8::kPromiseResolveAfterResolved;
+ using v8::Local;
+ using v8::Message;
++using v8::MicrotasksScope;
+ using v8::Number;
+ using v8::Object;
+ using v8::Promise;
+@@ -43,7 +44,7 @@ static void EnqueueMicrotask(const FunctionCallbackInfo<Value>& args) {
+ bool RunNextTicksNative(Environment* env) {
+   TickInfo* tick_info = env->tick_info();
+   if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())
+-    env->isolate()->RunMicrotasks();
++    MicrotasksScope::PerformCheckpoint(env->isolate());
+   if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())
+     return true;
+ 
+@@ -54,7 +55,7 @@ bool RunNextTicksNative(Environment* env) {
+ }
+ 
+ static void RunMicrotasks(const FunctionCallbackInfo<Value>& args) {
+-  args.GetIsolate()->RunMicrotasks();
++  MicrotasksScope::PerformCheckpoint(args.GetIsolate());
+ }
+ 
+ static void SetTickCallback(const FunctionCallbackInfo<Value>& args) {

shell/browser/net/node_stream_loader.cc

@@ -91,10 +91,18 @@ void NodeStreamLoader::NotifyComplete(int result) {
 }
 
 void NodeStreamLoader::ReadMore() {
+  if (is_reading_) {
+    // Calling read() can trigger the "readable" event again, making this
+    // function re-entrant. If we're already reading, we don't want to start
+    // a nested read, so short-circuit.
+    return;
+  }
   is_reading_ = true;
+  auto weak = weak_factory_.GetWeakPtr();
   // buffer = emitter.read()
   v8::MaybeLocal<v8::Value> ret = node::MakeCallback(
       isolate_, emitter_.Get(isolate_), "read", 0, nullptr, {0, 0});
+  DCHECK(weak) << "We shouldn't have been destroyed when calling read()";
 
   // If there is no buffer read, wait until |readable| is emitted again.
   v8::Local<v8::Value> buffer;
@@ -110,13 +118,12 @@ void NodeStreamLoader::ReadMore() {
   // Write buffer to mojo pipe asyncronously.
   is_reading_ = false;
   is_writing_ = true;
-  producer_->Write(
+  producer_->Write(std::make_unique<mojo::StringDataSource>(
-      std::make_unique<mojo::StringDataSource>(
+                       base::StringPiece(node::Buffer::Data(buffer),
-          base::StringPiece(node::Buffer::Data(buffer),
+                                         node::Buffer::Length(buffer)),
-                            node::Buffer::Length(buffer)),
+                       mojo::StringDataSource::AsyncWritingMode::
-          mojo::StringDataSource::AsyncWritingMode::
+                           STRING_STAYS_VALID_UNTIL_COMPLETION),
-              STRING_STAYS_VALID_UNTIL_COMPLETION),
+                   base::BindOnce(&NodeStreamLoader::DidWrite, weak));
-      base::BindOnce(&NodeStreamLoader::DidWrite, weak_factory_.GetWeakPtr()));
 }
 
 void NodeStreamLoader::DidWrite(MojoResult result) {

spec-main/api-session-spec.ts

@@ -240,17 +240,16 @@ describe('session module', () => {
       const port = (downloadServer.address() as AddressInfo).port
       const url = `http://127.0.0.1:${port}/`
 
-      const downloadPrevented: Promise<Electron.DownloadItem> = new Promise(resolve => {
+      const downloadPrevented: Promise<{itemUrl: string, itemFilename: string, item: Electron.DownloadItem}> = new Promise(resolve => {
         w.webContents.session.once('will-download', function (e, item) {
           e.preventDefault()
-          resolve(item)
+          resolve({itemUrl: item.getURL(), itemFilename: item.getFilename(), item})
         })
       })
       w.loadURL(url)
-      const item = await downloadPrevented
+      const {item, itemUrl, itemFilename} = await downloadPrevented
-      expect(item.getURL()).to.equal(url)
+      expect(itemUrl).to.equal(url)
-      expect(item.getFilename()).to.equal('mockFile.txt')
+      expect(itemFilename).to.equal('mockFile.txt')
-      await new Promise(setImmediate)
       expect(() => item.getURL()).to.throw('Object has been destroyed')
     })
   })

spec-main/api-web-contents-spec.ts

@@ -359,7 +359,6 @@ describe('webContents module', () => {
 
       // For some reason we have to wait for two focused events...?
       await emittedOnce(w.webContents, 'devtools-focused')
-      await emittedOnce(w.webContents, 'devtools-focused')
 
       expect(() => { webContents.getFocusedWebContents() }).to.not.throw()
 
@@ -444,9 +443,10 @@ describe('webContents module', () => {
       await focused
       expect(w.isFocused()).to.be.true()
       w.webContents.openDevTools({ mode: 'detach', activate: true })
-      await emittedOnce(w.webContents, 'devtools-focused')
+      await Promise.all([
-      await emittedOnce(w.webContents, 'devtools-focused')
+        emittedOnce(w.webContents, 'devtools-opened'),
-      await emittedOnce(w.webContents, 'devtools-opened')
+        emittedOnce(w.webContents, 'devtools-focused'),
+      ])
       await new Promise(resolve => setTimeout(resolve, 0))
       expect(w.isFocused()).to.be.false()
     })
@@ -572,8 +572,6 @@ describe('webContents module', () => {
 
         const [, zoomDirection] = await emittedOnce(w.webContents, 'zoom-changed')
         expect(zoomDirection).to.equal(zoomingIn ? 'in' : 'out')
-        // Apparently we get two zoom-changed events??
-        await emittedOnce(w.webContents, 'zoom-changed')
       }
 
       await testZoomChanged({ zoomingIn: true })