Skip to content

Commit

Permalink
feat: Fix ODA+ logging crash
Browse files Browse the repository at this point in the history
Bug: b/338268078
Change-Id: Iba8039d2f48356fc57f17998aa437448ec418138
GitOrigin-RevId: 21725678e5ce4a7a93bb19f42a6ce9bb3cf581e9
  • Loading branch information
Privacy Sandbox Team authored and copybara-github committed Dec 18, 2024
1 parent bfb0fa6 commit d616bf6
Show file tree
Hide file tree
Showing 8 changed files with 20 additions and 139 deletions.
8 changes: 7 additions & 1 deletion src/roma/config/type_converter.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,14 @@ struct TypeConverter<std::string> {
if (val.IsEmpty() || !val->IsString()) {
return false;
}
v8::HandleScope scope(isolate);
v8::TryCatch trycatch(isolate);

v8::Local<v8::String> str;
if (!val->ToString(isolate->GetCurrentContext()).ToLocal(&str)) {
return false;
}

v8::Local<v8::String> str = v8::Local<v8::String>::Cast(val);
const size_t len = str->Utf8Length(isolate);
out->resize(len);
str->WriteUtf8(isolate, &(*out)[0], len, nullptr,
Expand Down
2 changes: 1 addition & 1 deletion src/roma/roma_service/wasm_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ void LoggingFunction(absl::LogSeverity severity,
LOG(LEVEL(severity)) << msg;
}

TEST(WasmTest, DISABLED_CanLogFromInlineWasmCode) {
TEST(WasmTest, CanLogFromInlineWasmCode) {
Config config;
config.number_of_workers = 2;
config.SetLoggingFunction(LoggingFunction);
Expand Down
1 change: 0 additions & 1 deletion src/roma/sandbox/js_engine/v8_engine/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ cc_library(

cc_library(
name = "v8_console",
srcs = ["v8_console.cc"],
hdrs = ["v8_console.h"],
copts = [
"-Wno-macro-redefined", # v8 and absl define redundant log macros
Expand Down
101 changes: 0 additions & 101 deletions src/roma/sandbox/js_engine/v8_engine/v8_console.cc

This file was deleted.

27 changes: 5 additions & 22 deletions src/roma/sandbox/js_engine/v8_engine/v8_console.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,37 +40,20 @@
// clang-format on

namespace google::scp::roma::sandbox::js_engine::v8_js_engine {

// No-op implementation of the V8Console, allowing for logging from inline WASM
class V8Console : public v8::debug::ConsoleDelegate {
private:
using LogFunctionHandler =
absl::AnyInvocable<absl::Status(std::string_view, std::string_view,
google::scp::roma::logging::LogOptions)>;

public:
explicit V8Console(absl::Nonnull<v8::Isolate*> isolate,
LogFunctionHandler handle_log_func_);
V8Console() = default;
~V8Console() override = default;

void SetIds(std::string_view uuid, std::string_view id);
void SetMinLogLevel(absl::LogSeverity severity);

private:
void Log(const v8::debug::ConsoleCallArguments& args,
const v8::debug::ConsoleContext&) override;
const v8::debug::ConsoleContext&) override {};
void Warn(const v8::debug::ConsoleCallArguments& args,
const v8::debug::ConsoleContext&) override;
const v8::debug::ConsoleContext&) override {};
void Error(const v8::debug::ConsoleCallArguments& args,
const v8::debug::ConsoleContext&) override;

void HandleLog(const v8::debug::ConsoleCallArguments& args,
std::string_view function_name);

v8::Isolate* isolate_;
std::string invocation_req_uuid_;
std::string invocation_req_id_;
absl::LogSeverity min_log_level_ = absl::LogSeverity::kInfo;
LogFunctionHandler handle_log_func_;
const v8::debug::ConsoleContext&) override {};
};

} // namespace google::scp::roma::sandbox::js_engine::v8_js_engine
Expand Down
12 changes: 3 additions & 9 deletions src/roma/sandbox/js_engine/v8_engine/v8_js_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -453,22 +453,16 @@ std::unique_ptr<V8IsolateWrapper> V8JsEngine::CreateIsolate(
isolate->SetFatalErrorHandler(FatalErrorCallback);
isolate->AddGCPrologueCallback(GCPrologueCallback);
isolate->AddGCEpilogueCallback(GCEpilogueCallback);
v8::debug::SetConsoleDelegate(isolate, console());
return V8IsolateFactory::Create(isolate, std::move(allocator),
enable_profilers_);
}

V8Console* V8JsEngine::console(v8::Isolate* isolate)
ABSL_LOCKS_EXCLUDED(console_mutex_) {
V8Console* V8JsEngine::console() ABSL_LOCKS_EXCLUDED(console_mutex_) {
absl::MutexLock lock(&console_mutex_);
auto handle_log_func = [this](std::string_view function_name,
std::string_view msg, LogOptions log_options) {
return HandleLog(function_name, msg, std::move(log_options));
};

if (console_ == nullptr) {
console_ = std::make_unique<V8Console>(isolate, handle_log_func);
console_ = std::make_unique<V8Console>();
}

return console_.get();
}

Expand Down
2 changes: 1 addition & 1 deletion src/roma/sandbox/js_engine/v8_engine/v8_js_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ class V8JsEngine : public JsEngine {
/// timeouts the execution in set time.
std::unique_ptr<roma::worker::ExecutionWatchDog> execution_watchdog_{nullptr};

V8Console* console(v8::Isolate* isolate) ABSL_LOCKS_EXCLUDED(console_mutex_);
V8Console* console() ABSL_LOCKS_EXCLUDED(console_mutex_);
std::unique_ptr<V8Console> console_ ABSL_GUARDED_BY(console_mutex_);
absl::Mutex console_mutex_;
const bool skip_v8_cleanup_;
Expand Down
6 changes: 3 additions & 3 deletions src/roma/worker/execution_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ absl::Status ExecutionUtils::OverrideConsoleLog(v8::Isolate* isolate,
v8::Local<v8::Context> context(isolate->GetCurrentContext());

constexpr auto js_code = R"(
console.log = ROMA_LOG;
console.warn = ROMA_WARN;
console.error = ROMA_ERROR;
console.log = (...inputs) => ROMA_LOG(inputs.join(' '));
console.warn = (...inputs) => ROMA_WARN(inputs.join(' '));
console.error = (...inputs) => ROMA_ERROR(inputs.join(' '));
)";
v8::Local<v8::String> source =
v8::String::NewFromUtf8(isolate, js_code).ToLocalChecked();
Expand Down

0 comments on commit d616bf6

Please sign in to comment.