Repository: nodejs/node
Pull request: src: handle errors correctly in webstorage


Changed files:
filename: src/node_webstorage.cc
patch: @@ -24,12 +24,16 @@ using v8::IndexedPropertyHandlerConfiguration;
 using v8::Integer;
 using v8::Intercepted;
 using v8::Isolate;
+using v8::JustVoid;
 using v8::Local;
 using v8::Map;
 using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Name;
 using v8::NamedPropertyHandlerConfiguration;
+using v8::NewStringType;
+using v8::Nothing;
+using v8::Null;
 using v8::Object;
 using v8::PropertyAttribute;
 using v8::PropertyCallbackInfo;
@@ -96,7 +100,7 @@ void Storage::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackField("location", location_);
 }
 
-bool Storage::Open() {
+Maybe<void> Storage::Open() {
   static const int kCurrentSchemaVersion = 1;
   static constexpr std::string_view get_schema_version_sql =
       "SELECT schema_version FROM nodejs_webstorage_state";
@@ -161,30 +165,31 @@ bool Storage::Open() {
 
   sqlite3* db = db_.get();
   if (db != nullptr) {
-    return true;
+    return JustVoid();
   }
 
   int r = sqlite3_open(location_.c_str(), &db);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
   r = sqlite3_exec(db, init_sql_v0.data(), 0, 0, nullptr);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
 
   // Get the current schema version, used to determine schema migrations.
   sqlite3_stmt* s = nullptr;
   r = sqlite3_prepare_v2(
       db, get_schema_version_sql.data(), get_schema_version_sql.size(), &s, 0);
   r = sqlite3_exec(db, init_sql_v0.data(), 0, 0, nullptr);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
   auto stmt = stmt_unique_ptr(s);
-  CHECK_ERROR_OR_THROW(env(), sqlite3_step(stmt.get()), SQLITE_ROW, false);
+  CHECK_ERROR_OR_THROW(
+      env(), sqlite3_step(stmt.get()), SQLITE_ROW, Nothing<void>());
   CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_INTEGER);
   int schema_version = sqlite3_column_int(stmt.get(), 0);
   stmt = nullptr;  // Force finalization.
 
   if (schema_version > kCurrentSchemaVersion) {
     THROW_ERR_INVALID_STATE(
         env(), "localStorage was created with a newer version of Node.js");
-    return false;
+    return Nothing<void>();
   }
 
   if (schema_version < kCurrentSchemaVersion) {
@@ -193,11 +198,11 @@ bool Storage::Open() {
         "UPDATE nodejs_webstorage_state SET schema_version = " +
         std::to_string(kCurrentSchemaVersion) + ";";
     r = sqlite3_exec(db, set_user_version_sql.c_str(), 0, 0, nullptr);
-    CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+    CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
   }
 
   db_ = conn_unique_ptr(db);
-  return true;
+  return JustVoid();
 }
 
 void Storage::New(const FunctionCallbackInfo<Value>& args) {
@@ -222,9 +227,9 @@ void Storage::New(const FunctionCallbackInfo<Value>& args) {
   new Storage(env, args.This(), location.ToStringView());
 }
 
-void Storage::Clear() {
-  if (!Open()) {
-    return;
+Maybe<void> Storage::Clear() {
+  if (!Open().IsJust()) {
+    return Nothing<void>();
   }
 
   static constexpr std::string_view sql = "DELETE FROM nodejs_webstorage";
@@ -233,13 +238,15 @@ void Storage::Clear() {
       env(),
       sqlite3_prepare_v2(db_.get(), sql.data(), sql.size(), &s, 0),
       SQLITE_OK,
-      void());
+      Nothing<void>());
   auto stmt = stmt_unique_ptr(s);
-  CHECK_ERROR_OR_THROW(env(), sqlite3_step(stmt.get()), SQLITE_DONE, void());
+  CHECK_ERROR_OR_THROW(
+      env(), sqlite3_step(stmt.get()), SQLITE_DONE, Nothing<void>());
+  return JustVoid();
 }
 
-Local<Array> Storage::Enumerate() {
-  if (!Open()) {
+MaybeLocal<Array> Storage::Enumerate() {
+  if (!Open().IsJust()) {
     return Local<Array>();
   }
 
@@ -256,7 +263,7 @@ Local<Array> Storage::Enumerate() {
     if (!String::NewFromTwoByte(env()->isolate(),
                                 reinterpret_cast<const uint16_t*>(
                                     sqlite3_column_blob(stmt.get(), 0)),
-                                v8::NewStringType::kNormal,
+                                NewStringType::kNormal,
                                 size)
              .ToLocal(&value)) {
       return Local<Array>();
@@ -267,8 +274,8 @@ Local<Array> Storage::Enumerate() {
   return Array::New(env()->isolate(), values.data(), values.size());
 }
 
-Local<Value> Storage::Length() {
-  if (!Open()) {
+MaybeLocal<Value> Storage::Length() {
+  if (!Open().IsJust()) {
     return {};
   }
 
@@ -285,14 +292,13 @@ Local<Value> Storage::Length() {
   return Integer::New(env()->isolate(), result);
 }
 
-Local<Value> Storage::Load(Local<Name> key) {
+MaybeLocal<Value> Storage::Load(Local<Name> key) {
   if (key->IsSymbol()) {
     auto symbol_map = symbols_.Get(env()->isolate());
-    MaybeLocal<Value> result = symbol_map->Get(env()->context(), key);
-    return result.FromMaybe(Local<Value>());
+    return symbol_map->Get(env()->context(), key);
   }
 
-  if (!Open()) {
+  if (!Open().IsJust()) {
     return {};
   }
 
@@ -306,28 +312,26 @@ Local<Value> Storage::Load(Local<Name> key) {
   auto key_size = utf16key.length() * sizeof(uint16_t);
   r = sqlite3_bind_blob(stmt.get(), 1, utf16key.out(), key_size, SQLITE_STATIC);
   CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Local<Value>());
-  auto value = Local<Value>();
   r = sqlite3_step(stmt.get());
   if (r == SQLITE_ROW) {
     CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB);
     auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
-    if (!String::NewFromTwoByte(env()->isolate(),
-                                reinterpret_cast<const uint16_t*>(
-                                    sqlite3_column_blob(stmt.get(), 0)),
-                                v8::NewStringType::kNormal,
-                                size)
-             .ToLocal(&value)) {
-      return {};
-    }
+    return String::NewFromTwoByte(env()->isolate(),
+                                  reinterpret_cast<const uint16_t*>(
+                                      sqlite3_column_blob(stmt.get(), 0)),
+                                  NewStringType::kNormal,
+                                  size)
+        .As<Value>();
   } else if (r != SQLITE_DONE) {
     THROW_SQLITE_ERROR(env(), r);
+    return {};
+  } else {
+    return Null(env()->isolate());
   }
-
-  return value;
 }
 
-Local<Value> Storage::LoadKey(const int index) {
-  if (!Open()) {
+MaybeLocal<Value> Storage::LoadKey(const int index) {
+  if (!Open().IsJust()) {
     return {};
   }
 
@@ -340,65 +344,64 @@ Local<Value> Storage::LoadKey(const int index) {
   r = sqlite3_bind_int(stmt.get(), 1, index);
   CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Local<Value>());
 
-  auto value = Local<Value>();
   r = sqlite3_step(stmt.get());
   if (r == SQLITE_ROW) {
     CHECK(sqlite3_column_type(stmt.get(), 0) == SQLITE_BLOB);
     auto size = sqlite3_column_bytes(stmt.get(), 0) / sizeof(uint16_t);
-    if (!String::NewFromTwoByte(env()->isolate(),
-                                reinterpret_cast<const uint16_t*>(
-                                    sqlite3_column_blob(stmt.get(), 0)),
-                                v8::NewStringType::kNormal,
-                                size)
-             .ToLocal(&value)) {
-      return {};
-    }
+    return String::NewFromTwoByte(env()->isolate(),
+                                  reinterpret_cast<const uint16_t*>(
+                                      sqlite3_column_blob(stmt.get(), 0)),
+                                  NewStringType::kNormal,
+                                  size)
+        .As<Value>();
   } else if (r != SQLITE_DONE) {
     THROW_SQLITE_ERROR(env(), r);
+    return {};
+  } else {
+    return Null(env()->isolate());
   }
-
-  return value;
 }
 
-bool Storage::Remove(Local<Name> key) {
+Maybe<void> Storage::Remove(Local<Name> key) {
   if (key->IsSymbol()) {
     auto symbol_map = symbols_.Get(env()->isolate());
     Maybe<bool> result = symbol_map->Delete(env()->context(), key);
-    return !result.IsNothing();
+    return result.IsNothing() ? Nothing<void>() : JustVoid();
   }
 
-  if (!Open()) {
-    return false;
+  if (!Open().IsJust()) {
+    return Nothing<void>();
   }
 
   static constexpr std::string_view sql =
       "DELETE FROM nodejs_webstorage WHERE key = ?";
   sqlite3_stmt* s = nullptr;
   int r = sqlite3_prepare_v2(db_.get(), sql.data(), sql.size(), &s, 0);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
   auto stmt = stmt_unique_ptr(s);
   TwoByteValue utf16key(env()->isolate(), key);
   auto key_size = utf16key.length() * sizeof(uint16_t);
   r = sqlite3_bind_blob(stmt.get(), 1, utf16key.out(), key_size, SQLITE_STATIC);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
-  CHECK_ERROR_OR_THROW(env(), sqlite3_step(stmt.get()), SQLITE_DONE, false);
-  return true;
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
+  CHECK_ERROR_OR_THROW(
+      env(), sqlite3_step(stmt.get()), SQLITE_DONE, Nothing<void>());
+  return JustVoid();
 }
 
-bool Storage::Store(Local<Name> key, Local<Value> value) {
+Maybe<void> Storage::Store(Local<Name> key, Local<Value> value) {
   if (key->IsSymbol()) {
     auto symbol_map = symbols_.Get(env()->isolate());
     MaybeLocal<Map> result = symbol_map->Set(env()->context(), key, value);
-    return !result.IsEmpty();
+    return result.IsEmpty() ? Nothing<void>() : JustVoid();
   }
 
   Local<String> val;
   if (!value->ToString(env()->context()).ToLocal(&val)) {
-    return false;
+    return Nothing<void>();
   }
 
-  if (!Open()) {
-    return false;
+  if (!Open().IsJust()) {
+    return Nothing<void>();
   }
 
   static constexpr std::string_view sql =
@@ -409,23 +412,23 @@ bool Storage::Store(Local<Name> key, Local<Value> value) {
   TwoByteValue utf16key(env()->isolate(), key);
   TwoByteValue utf16val(env()->isolate(), val);
   int r = sqlite3_prepare_v2(db_.get(), sql.data(), sql.size(), &s, 0);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
   auto stmt = stmt_unique_ptr(s);
   auto key_size = utf16key.length() * sizeof(uint16_t);
   r = sqlite3_bind_blob(stmt.get(), 1, utf16key.out(), key_size, SQLITE_STATIC);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
   auto val_size = utf16val.length() * sizeof(uint16_t);
   r = sqlite3_bind_blob(stmt.get(), 2, utf16val.out(), val_size, SQLITE_STATIC);
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, false);
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
 
   r = sqlite3_step(stmt.get());
   if (r == SQLITE_CONSTRAINT) {
     ThrowQuotaExceededException(env()->context());
-    return false;
+    return Nothing<void>();
   }
 
-  CHECK_ERROR_OR_THROW(env(), r, SQLITE_DONE, false);
-  return true;
+  CHECK_ERROR_OR_THROW(env(), r, SQLITE_DONE, Nothing<void>());
+  return JustVoid();
 }
 
 static MaybeLocal<String> Uint32ToName(Local<Context> context, uint32_t index) {
@@ -453,12 +456,11 @@ static void GetItem(const FunctionCallbackInfo<Value>& info) {
     return;
   }
 
-  Local<Value> result = storage->Load(prop);
-  if (result.IsEmpty()) {
-    info.GetReturnValue().SetNull();
-  } else {
-    info.GetReturnValue().Set(result);
+  Local<Value> result;
+  if (!storage->Load(prop).ToLocal(&result)) {
+    return;
   }
+  info.GetReturnValue().Set(result);
 }
 
 static void Key(const FunctionCallbackInfo<Value>& info) {
@@ -481,10 +483,8 @@ static void Key(const FunctionCallbackInfo<Value>& info) {
     return;
   }
 
-  Local<Value> result = storage->LoadKey(index);
-  if (result.IsEmpty()) {
-    info.GetReturnValue().SetNull();
-  } else {
+  Local<Value> result;
+  if (storage->LoadKey(index).ToLocal(&result)) {
     info.GetReturnValue().Set(result);
   }
 }
@@ -555,11 +555,9 @@ static Intercepted StorageGetter(Local<Name> property,
 
   Storage* storage;
   ASSIGN_OR_RETURN_UNWRAP(&storage, info.This(), Intercepted::kNo);
-  Local<Value> result = storage->Load(property);
+  Local<Value> result;
 
-  if (result.IsEmpty()) {
-    info.GetReturnValue().SetUndefined();
-  } else {
+  if (storage->Load(property).ToLocal(&result) && !result->IsNull()) {
     info.GetReturnValue().Set(result);
   }
 
@@ -572,7 +570,7 @@ static Intercepted StorageSetter(Local<Name> property,
   Storage* storage;
   ASSIGN_OR_RETURN_UNWRAP(&storage, info.This(), Intercepted::kNo);
 
-  if (!storage->Store(property, value)) {
+  if (storage->Store(property, value).IsNothing()) {
     info.GetReturnValue().SetFalse();
   }
 
@@ -587,8 +585,8 @@ static Intercepted StorageQuery(Local<Name> property,
 
   Storage* storage;
   ASSIGN_OR_RETURN_UNWRAP(&storage, info.This(), Intercepted::kNo);
-  Local<Value> result = storage->Load(property);
-  if (result.IsEmpty()) {
+  Local<Value> result;
+  if (!storage->Load(property).ToLocal(&result) || result->IsNull()) {
     return Intercepted::kNo;
   }
 
@@ -601,17 +599,19 @@ static Intercepted StorageDeleter(Local<Name> property,
   Storage* storage;
   ASSIGN_OR_RETURN_UNWRAP(&storage, info.This(), Intercepted::kNo);
 
-  if (storage->Remove(property)) {
-    info.GetReturnValue().Set(true);
-  }
+  info.GetReturnValue().Set(storage->Remove(property).IsJust());
 
   return Intercepted::kYes;
 }
 
 static void StorageEnumerator(const PropertyCallbackInfo<Array>& info) {
   Storage* storage;
   ASSIGN_OR_RETURN_UNWRAP(&storage, info.This());
-  info.GetReturnValue().Set(storage->Enumerate());
+  Local<Array> result;
+  if (!storage->Enumerate().ToLocal(&result)) {
+    return;
+  }
+  info.GetReturnValue().Set(result);
 }
 
 static Intercepted StorageDefiner(Local<Name> property,
@@ -697,7 +697,11 @@ static Intercepted IndexedDefiner(uint32_t index,
 static void StorageLengthGetter(const FunctionCallbackInfo<Value>& info) {
   Storage* storage;
   ASSIGN_OR_RETURN_UNWRAP(&storage, info.This());
-  info.GetReturnValue().Set(storage->Length());
+  Local<Value> result;
+  if (!storage->Length().ToLocal(&result)) {
+    return;
+  }
+  info.GetReturnValue().Set(result);
 }
 
 static void Initialize(Local<Object> target,

filename: src/node_webstorage.h
patch: @@ -33,19 +33,19 @@ class Storage : public BaseObject {
   void MemoryInfo(MemoryTracker* tracker) const override;
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
 
-  void Clear();
-  v8::Local<v8::Array> Enumerate();
-  v8::Local<v8::Value> Length();
-  v8::Local<v8::Value> Load(v8::Local<v8::Name> key);
-  v8::Local<v8::Value> LoadKey(const int index);
-  bool Remove(v8::Local<v8::Name> key);
-  bool Store(v8::Local<v8::Name> key, v8::Local<v8::Value> value);
+  v8::Maybe<void> Clear();
+  v8::MaybeLocal<v8::Array> Enumerate();
+  v8::MaybeLocal<v8::Value> Length();
+  v8::MaybeLocal<v8::Value> Load(v8::Local<v8::Name> key);
+  v8::MaybeLocal<v8::Value> LoadKey(const int index);
+  v8::Maybe<void> Remove(v8::Local<v8::Name> key);
+  v8::Maybe<void> Store(v8::Local<v8::Name> key, v8::Local<v8::Value> value);
 
   SET_MEMORY_INFO_NAME(Storage)
   SET_SELF_SIZE(Storage)
 
  private:
-  bool Open();
+  v8::Maybe<void> Open();
 
   ~Storage() override;
   std::string location_;