Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions Libraries/LibWebView/ProcessManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ StringView process_name_from_type(ProcessType type)
}

ProcessManager::ProcessManager()
: on_process_exited([](Process&&) {})
: on_process_added([](Process&) {})
, on_process_exited([](Process&&) {})
, m_process_monitor(ProcessMonitor([this](pid_t pid) {
if (auto process = remove_process(pid); process.has_value())
on_process_exited(process.release_value());
Expand All @@ -66,24 +67,32 @@ ProcessManager::ProcessManager()

Optional<Process&> ProcessManager::find_process(pid_t pid)
{
verify_event_loop();
return m_processes.get(pid);
}

void ProcessManager::add_process(WebView::Process&& process)
{
Threading::MutexLocker locker { m_lock };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mutex seems dubious beforehand as well considering it does not protect find_process properly, but it does not seem correct to remove the mutex here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HashMap owns the Process object lifetimes, and find_process() returns a borrowed reference into that storage. We can't guarantee its lifetime to another thread.

I would just replace the misleading mutex and locks with event loop thread-affinity asserts, and revisit the design if non-event-loop callers are ever needed.


verify_event_loop();
auto pid = process.pid();
on_process_added(process);
auto result = m_processes.set(pid, move(process));
VERIFY(result == AK::HashSetResult::InsertedNewEntry);
m_statistics.processes.append(make<Core::Platform::ProcessInfo>(pid));
m_process_monitor.add_process(pid);
}

void ProcessManager::for_each_process(Function<void(Process&)> callback)
{
verify_event_loop();
for (auto& entry : m_processes)
callback(entry.value);
}

#if defined(AK_OS_MACH)
void ProcessManager::set_process_mach_port(pid_t pid, Core::MachPort&& port)
{
Threading::MutexLocker locker { m_lock };
verify_event_loop();
for (auto const& info : m_statistics.processes) {
if (info->pid == pid) {
info->child_task_port = move(port);
Expand All @@ -95,7 +104,7 @@ void ProcessManager::set_process_mach_port(pid_t pid, Core::MachPort&& port)

Optional<Process> ProcessManager::remove_process(pid_t pid)
{
Threading::MutexLocker locker { m_lock };
verify_event_loop();
m_statistics.processes.remove_first_matching([&](auto const& info) {
return (info->pid == pid);
});
Expand All @@ -104,17 +113,17 @@ Optional<Process> ProcessManager::remove_process(pid_t pid)

void ProcessManager::update_all_process_statistics()
{
Threading::MutexLocker locker { m_lock };
verify_event_loop();
(void)update_process_statistics(m_statistics);
}

JsonValue ProcessManager::serialize_json()
{
Threading::MutexLocker locker { m_lock };
verify_event_loop();
JsonArray serialized;

m_statistics.for_each_process([&](auto const& process) {
auto& process_handle = find_process(process.pid).value();
auto& process_handle = m_processes.get(process.pid).value();

auto type = WebView::process_name_from_type(process_handle.type());
auto const& title = process_handle.title();
Expand All @@ -134,4 +143,10 @@ JsonValue ProcessManager::serialize_json()
return serialized;
}

void ProcessManager::verify_event_loop() const
{
if (Core::EventLoop::is_running())
VERIFY(&Core::EventLoop::current() == m_creation_event_loop);
}

}
9 changes: 7 additions & 2 deletions Libraries/LibWebView/ProcessManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@

#pragma once

#include <AK/Function.h>
#include <AK/JsonValue.h>
#include <AK/Types.h>
#include <LibCore/EventLoop.h>
#include <LibCore/Platform/ProcessStatistics.h>
#include <LibThreading/Mutex.h>
#include <LibWebView/Forward.h>
#include <LibWebView/Process.h>
#include <LibWebView/ProcessMonitor.h>
Expand All @@ -27,6 +28,7 @@ class WEBVIEW_API ProcessManager {
ProcessManager();

void add_process(Process&&);
void for_each_process(Function<void(Process&)>);
Optional<Process> remove_process(pid_t);
Optional<Process&> find_process(pid_t);

Expand All @@ -37,13 +39,16 @@ class WEBVIEW_API ProcessManager {
void update_all_process_statistics();
JsonValue serialize_json();

Function<void(Process&)> on_process_added; // test-web
Function<void(Process&&)> on_process_exited;

private:
void verify_event_loop() const;

Core::Platform::ProcessStatistics m_statistics;
HashMap<pid_t, Process> m_processes;
ProcessMonitor m_process_monitor;
Threading::Mutex m_lock;
Core::EventLoop* m_creation_event_loop { &Core::EventLoop::current() };
};

}
1 change: 0 additions & 1 deletion Tests/LibWeb/test-web/Application.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ void Application::create_platform_arguments(Core::ArgsParser& args_parser)
args_parser.add_option(test_globs, "Only run tests matching the given glob", "filter", 'f', "glob");
args_parser.add_option(python_executable_path, "Path to python3", "python-executable", 'P', "path");
args_parser.add_option(dump_gc_graph, "Dump GC graph", "dump-gc-graph", 'G');
args_parser.add_option(debug_timeouts, "Capture backtrace on timeouts (see test-dumps html -> Timeouts -> stderr)", "debug-timeouts");
args_parser.add_option(fail_fast, "Abort on first failure/timeout/crash (offers debugger attach on timeout)", "fail-fast");

args_parser.add_option(repeat_count, "Repeat all matched tests N times", "repeat", 0, "n");
Expand Down
3 changes: 2 additions & 1 deletion Tests/LibWeb/test-web/Application.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <AK/ByteString.h>
#include <AK/Error.h>
#include <AK/String.h>
#include <AK/Vector.h>
#include <LibWebView/Application.h>

Expand Down Expand Up @@ -37,9 +38,9 @@ class Application : public WebView::Application {
Vector<ByteString> test_globs;

ByteString python_executable_path;
String invocation_command_line;

bool dump_gc_graph { false };
bool debug_timeouts { false };
bool fail_fast { false };
size_t repeat_count { 1 };
bool test_dry_run { false };
Expand Down
2 changes: 2 additions & 0 deletions Tests/LibWeb/test-web/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
set(SOURCES
Application.cpp
CaptureFile.cpp
Debug.cpp
Fixture.cpp
Fuzzy.cpp
TestRunCapture.cpp
TestWebView.cpp
main.cpp
)
Expand Down
Loading
Loading