fix: check the result when calling js function (backport: 5-0-x) (#17481

) * fix: check the result when calling js function * test: should not crash when callback returns nothing
electron · Mar 20, 2019 · 821b88e · 821b88e
1 parent d909345
commit 821b88e
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 18 deletions.
diff --git a/atom/common/native_mate_converters/callback.h b/atom/common/native_mate_converters/callback.h
@@ -55,11 +55,12 @@ struct V8FunctionInvoker<v8::Local<v8::Value>(ArgTypes...)> {
     v8::Local<v8::Context> context = holder->CreationContext();
     v8::Context::Scope context_scope(context);
     std::vector<v8::Local<v8::Value>> args{ConvertToV8(isolate, raw)...};
-    v8::Local<v8::Value> ret(holder
-                                 ->Call(context, holder, args.size(),
-                                        args.empty() ? nullptr : &args.front())
-                                 .ToLocalChecked());
-    return handle_scope.Escape(ret);
+    v8::MaybeLocal<v8::Value> ret = holder->Call(
+        context, holder, args.size(), args.empty() ? nullptr : &args.front());
+    if (ret.IsEmpty())
+      return v8::Undefined(isolate);
+    else
+      return handle_scope.Escape(ret.ToLocalChecked());
   }
 };
 
@@ -81,7 +82,7 @@ struct V8FunctionInvoker<void(ArgTypes...)> {
     holder
         ->Call(context, holder, args.size(),
                args.empty() ? nullptr : &args.front())
-        .ToLocalChecked();
+        .IsEmpty();
   }
 };
 

diff --git a/atom/common/native_mate_converters/v8_value_converter.cc b/atom/common/native_mate_converters/v8_value_converter.cc
@@ -334,11 +334,10 @@ std::unique_ptr<base::Value> V8ValueConverter::FromV8ValueImpl(
                                           v8::NewStringType::kNormal)
                       .ToLocalChecked());
     if (toISOString->IsFunction()) {
-      v8::Local<v8::Value> result = toISOString.As<v8::Function>()
-                                        ->Call(context, val, 0, nullptr)
-                                        .ToLocalChecked();
+      v8::MaybeLocal<v8::Value> result =
+          toISOString.As<v8::Function>()->Call(context, val, 0, nullptr);
       if (!result.IsEmpty()) {
-        v8::String::Utf8Value utf8(isolate, result);
+        v8::String::Utf8Value utf8(isolate, result.ToLocalChecked());
         return std::make_unique<base::Value>(std::string(*utf8, utf8.length()));
       }
     }

diff --git a/atom/renderer/api/atom_api_spell_check_client.cc b/atom/renderer/api/atom_api_spell_check_client.cc
@@ -215,7 +215,7 @@ void SpellCheckClient::SpellCheckWords(
   v8::Local<v8::Value> args[] = {mate::ConvertToV8(isolate_, words),
                                  templ->GetFunction(context).ToLocalChecked()};
   // Call javascript with the words and the callback function
-  scope.spell_check_->Call(context, scope.provider_, 2, args).ToLocalChecked();
+  scope.spell_check_->Call(context, scope.provider_, 2, args).IsEmpty();
 }
 
 // Returns whether or not the given string is a contraction.

diff --git a/native_mate/native_mate/wrappable.cc b/native_mate/native_mate/wrappable.cc
@@ -39,8 +39,7 @@ void WrappableBase::InitWith(v8::Isolate* isolate,
   // Call object._init if we have one.
   v8::Local<v8::Function> init;
   if (Dictionary(isolate, wrapper).Get("_init", &init))
-    init->Call(isolate->GetCurrentContext(), wrapper, 0, nullptr)
-        .ToLocalChecked();
+    init->Call(isolate->GetCurrentContext(), wrapper, 0, nullptr).IsEmpty();
 
   AfterInit(isolate);
 }

diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js
@@ -2442,17 +2442,26 @@ describe('BrowserWindow module', () => {
 
   describe('beginFrameSubscription method', () => {
     before(function () {
-      // This test is too slow, only test it on CI.
-      if (!isCI) {
-        this.skip()
-      }
-
       // FIXME These specs crash on Linux when run in a docker container
       if (isCI && process.platform === 'linux') {
         this.skip()
       }
     })
 
+    it('does not crash when callback returns nothing', (done) => {
+      w.loadFile(path.join(fixtures, 'api', 'frame-subscriber.html'))
+      w.webContents.on('dom-ready', () => {
+        w.webContents.beginFrameSubscription(function (data) {
+          // Pending endFrameSubscription to next tick can reliably reproduce
+          // a crash which happens when nothing is returned in the callback.
+          setTimeout(() => {
+            w.webContents.endFrameSubscription()
+            done()
+          })
+        })
+      })
+    })
+
     it('subscribes to frame updates', (done) => {
       let called = false
       w.loadFile(path.join(fixtures, 'api', 'frame-subscriber.html'))