Spreadsheet: Override `visit_edges()` and visit stored JS objects

...and don't let them leak out of their evaluation contexts.
Also keep the exceptions separate from the actual values.
This greatly reduces the number of assertions hit while entering random
data into a sheet.

11 files changed, 110 insertions, 43 deletions

diff --git a/Applications/Spreadsheet/Cell.cpp b/Applications/Spreadsheet/Cell.cpp
index 4db2682a59..2192ce3d66 100644
--- a/Applications/Spreadsheet/Cell.cpp
+++ b/Applications/Spreadsheet/Cell.cpp
@@ -110,11 +110,16 @@ void Cell::update_data(Badge<Sheet>)
     if (!m_dirty)
         return;
 
+    m_js_exception = {};
+
     if (m_dirty) {
         m_dirty = false;
         if (m_kind == Formula) {
-            if (!m_evaluated_externally)
-                m_evaluated_data = m_sheet->evaluate(m_data, this);
+            if (!m_evaluated_externally) {
+                auto [value, exception] = m_sheet->evaluate(m_data, this);
+                m_evaluated_data = value;
+                m_js_exception = move(exception);
+            }
         }
 
         for (auto& ref : m_referencing_cells) {
@@ -127,19 +132,25 @@ void Cell::update_data(Badge<Sheet>)
 
     m_evaluated_formats.background_color.clear();
     m_evaluated_formats.foreground_color.clear();
-    StringBuilder builder;
-    for (auto& fmt : m_conditional_formats) {
-        if (!fmt.condition.is_empty()) {
-            builder.clear();
-            builder.append("return (");
-            builder.append(fmt.condition);
-            builder.append(')');
-            auto value = m_sheet->evaluate(builder.string_view(), this);
-            if (value.to_boolean()) {
-                if (fmt.background_color.has_value())
-                    m_evaluated_formats.background_color = fmt.background_color;
-                if (fmt.foreground_color.has_value())
-                    m_evaluated_formats.foreground_color = fmt.foreground_color;
+    if (!m_js_exception) {
+        StringBuilder builder;
+        for (auto& fmt : m_conditional_formats) {
+            if (!fmt.condition.is_empty()) {
+                builder.clear();
+                builder.append("return (");
+                builder.append(fmt.condition);
+                builder.append(')');
+                auto [value, exception] = m_sheet->evaluate(builder.string_view(), this);
+                if (exception) {
+                    m_js_exception = move(exception);
+                } else {
+                    if (value.to_boolean()) {
+                        if (fmt.background_color.has_value())
+                            m_evaluated_formats.background_color = fmt.background_color;
+                        if (fmt.foreground_color.has_value())
+                            m_evaluated_formats.foreground_color = fmt.foreground_color;
+                    }
+                }
             }
         }
     }
@@ -193,6 +204,8 @@ void Cell::copy_from(const Cell& other)
     m_type_metadata = other.m_type_metadata;
     m_conditional_formats = other.m_conditional_formats;
     m_evaluated_formats = other.m_evaluated_formats;
+    if (!other.m_js_exception)
+        m_js_exception = other.m_js_exception;
 }
 
 }
diff --git a/Applications/Spreadsheet/Cell.h b/Applications/Spreadsheet/Cell.h
index 19e63e1328..2487a43d4e 100644
--- a/Applications/Spreadsheet/Cell.h
+++ b/Applications/Spreadsheet/Cell.h
@@ -67,6 +67,10 @@ struct Cell : public Weakable<Cell> {
     void set_data(String new_data);
     void set_data(JS::Value new_data);
     bool dirty() const { return m_dirty; }
+    void clear_dirty() { m_dirty = false; }
+
+    void set_exception(JS::Exception* exc) { m_js_exception = exc; }
+    JS::Exception* exception() const { return m_js_exception; }
 
     const String& data() const { return m_data; }
     const JS::Value& evaluated_data() const { return m_evaluated_data; }
@@ -117,6 +121,7 @@ private:
     bool m_evaluated_externally { false };
     String m_data;
     JS::Value m_evaluated_data;
+    JS::Exception* m_js_exception { nullptr };
     Kind m_kind { LiteralString };
     WeakPtr<Sheet> m_sheet;
     Vector<WeakPtr<Cell>> m_referencing_cells;
diff --git a/Applications/Spreadsheet/CellType/Date.cpp b/Applications/Spreadsheet/CellType/Date.cpp
index eb0937ed55..005620006f 100644
--- a/Applications/Spreadsheet/CellType/Date.cpp
+++ b/Applications/Spreadsheet/CellType/Date.cpp
@@ -27,6 +27,7 @@
 #include "Date.h"
 #include "../Cell.h"
 #include "../Spreadsheet.h"
+#include <AK/ScopeGuard.h>
 #include <LibCore/DateTime.h>
 
 namespace Spreadsheet {
@@ -42,6 +43,12 @@ DateCell::~DateCell()
 
 String DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const
 {
+    ScopeGuard propagate_exception { [&cell] {
+        if (auto exc = cell.sheet().interpreter().exception()) {
+            cell.sheet().interpreter().vm().clear_exception();
+            cell.set_exception(exc);
+        }
+    } };
     auto timestamp = js_value(cell, metadata);
     auto string = Core::DateTime::from_timestamp(timestamp.to_i32(cell.sheet().global_object())).to_string(metadata.format.is_empty() ? "%Y-%m-%d %H:%M:%S" : metadata.format.characters());
 
@@ -53,7 +60,8 @@ String DateCell::display(Cell& cell, const CellTypeMetadata& metadata) const
 
 JS::Value DateCell::js_value(Cell& cell, const CellTypeMetadata&) const
 {
-    auto value = cell.js_data().to_double(cell.sheet().global_object());
+    auto js_data = cell.js_data();
+    auto value = js_data.to_double(cell.sheet().global_object());
     return JS::Value(value / 1000); // Turn it to seconds
 }
 
diff --git a/Applications/Spreadsheet/CellType/Numeric.cpp b/Applications/Spreadsheet/CellType/Numeric.cpp
index b69ca1bf21..d58d48cd85 100644
--- a/Applications/Spreadsheet/CellType/Numeric.cpp
+++ b/Applications/Spreadsheet/CellType/Numeric.cpp
@@ -28,6 +28,7 @@
 #include "../Cell.h"
 #include "../Spreadsheet.h"
 #include "Format.h"
+#include <AK/ScopeGuard.h>
 
 namespace Spreadsheet {
 
@@ -42,6 +43,12 @@ NumericCell::~NumericCell()
 
 String NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const
 {
+    ScopeGuard propagate_exception { [&cell] {
+        if (auto exc = cell.sheet().interpreter().exception()) {
+            cell.sheet().interpreter().vm().clear_exception();
+            cell.set_exception(exc);
+        }
+    } };
     auto value = js_value(cell, metadata);
     String string;
     if (metadata.format.is_empty())
@@ -57,6 +64,12 @@ String NumericCell::display(Cell& cell, const CellTypeMetadata& metadata) const
 
 JS::Value NumericCell::js_value(Cell& cell, const CellTypeMetadata&) const
 {
+    ScopeGuard propagate_exception { [&cell] {
+        if (auto exc = cell.sheet().interpreter().exception()) {
+            cell.sheet().interpreter().vm().clear_exception();
+            cell.set_exception(exc);
+        }
+    } };
     return cell.js_data().to_number(cell.sheet().global_object());
 }
 
diff --git a/Applications/Spreadsheet/JSIntegration.cpp b/Applications/Spreadsheet/JSIntegration.cpp
index 7c94cda388..5feddc9bf2 100644
--- a/Applications/Spreadsheet/JSIntegration.cpp
+++ b/Applications/Spreadsheet/JSIntegration.cpp
@@ -87,6 +87,16 @@ void SheetGlobalObject::initialize()
     define_native_function("column_index", column_index, 1);
 }
 
+void SheetGlobalObject::visit_edges(Visitor& visitor)
+{
+    GlobalObject::visit_edges(visitor);
+    for (auto& it : m_sheet.cells()) {
+        if (it.value->exception())
+            visitor.visit(it.value->exception());
+        visitor.visit(it.value->evaluated_data());
+    }
+}
+
 JS_DEFINE_NATIVE_FUNCTION(SheetGlobalObject::parse_cell_name)
 {
     if (vm.argument_count() != 1) {
@@ -231,6 +241,13 @@ void WorkbookObject::initialize(JS::GlobalObject& global_object)
     define_native_function("sheet", sheet, 1);
 }
 
+void WorkbookObject::visit_edges(Visitor& visitor)
+{
+    Base::visit_edges(visitor);
+    for (auto& sheet : m_workbook.sheets())
+        visitor.visit(&sheet.global_object());
+}
+
 JS_DEFINE_NATIVE_FUNCTION(WorkbookObject::sheet)
 {
     if (vm.argument_count() != 1) {
diff --git a/Applications/Spreadsheet/JSIntegration.h b/Applications/Spreadsheet/JSIntegration.h
index 995694d8a1..4ad289e61f 100644
--- a/Applications/Spreadsheet/JSIntegration.h
+++ b/Applications/Spreadsheet/JSIntegration.h
@@ -50,6 +50,7 @@ public:
     JS_DECLARE_NATIVE_FUNCTION(column_arithmetic);
 
 private:
+    virtual void visit_edges(Visitor&) override;
     Sheet& m_sheet;
 };
 
@@ -66,6 +67,7 @@ public:
     JS_DECLARE_NATIVE_FUNCTION(sheet);
 
 private:
+    virtual void visit_edges(Visitor&) override;
     Workbook& m_workbook;
 };
 
diff --git a/Applications/Spreadsheet/Spreadsheet.cpp b/Applications/Spreadsheet/Spreadsheet.cpp
index 881dfb4abf..3ff03b0916 100644
--- a/Applications/Spreadsheet/Spreadsheet.cpp
+++ b/Applications/Spreadsheet/Spreadsheet.cpp
@@ -59,10 +59,11 @@ Sheet::Sheet(const StringView& name, Workbook& workbook)
 Sheet::Sheet(Workbook& workbook)
     : m_workbook(workbook)
 {
+    JS::DeferGC defer_gc(m_workbook.interpreter().heap());
     m_global_object = m_workbook.interpreter().heap().allocate_without_global_object<SheetGlobalObject>(*this);
-    m_global_object->initialize();
-    m_global_object->put("workbook", m_workbook.workbook_object());
-    m_global_object->put("thisSheet", m_global_object); // Self-reference is unfortunate, but required.
+    global_object().initialize();
+    global_object().put("workbook", m_workbook.workbook_object());
+    global_object().put("thisSheet", &global_object()); // Self-reference is unfortunate, but required.
 
     // Sadly, these have to be evaluated once per sheet.
     auto file_or_error = Core::File::open("/res/js/Spreadsheet/runtime.js", Core::IODevice::OpenMode::ReadOnly);
@@ -179,26 +180,26 @@ void Sheet::update(Cell& cell)
     }
 }
 
-JS::Value Sheet::evaluate(const StringView& source, Cell* on_behalf_of)
+Sheet::ValueAndException Sheet::evaluate(const StringView& source, Cell* on_behalf_of)
 {
     TemporaryChange cell_change { m_current_cell_being_evaluated, on_behalf_of };
+    ScopeGuard clear_exception { [&] { interpreter().vm().clear_exception(); } };
 
     auto parser = JS::Parser(JS::Lexer(source));
-    if (parser.has_errors())
-        return JS::js_undefined();
+    if (parser.has_errors() || interpreter().exception())
+        return { JS::js_undefined(), interpreter().exception() };
 
     auto program = parser.parse_program();
     interpreter().run(global_object(), program);
     if (interpreter().exception()) {
-        auto exc = interpreter().exception()->value();
-        interpreter().vm().clear_exception();
-        return exc;
+        auto exc = interpreter().exception();
+        return { JS::js_undefined(), exc };
     }
 
     auto value = interpreter().vm().last_value();
     if (value.is_empty())
-        return JS::js_undefined();
-    return value;
+        return { JS::js_undefined(), {} };
+    return { value, {} };
 }
 
 Cell* Sheet::at(const StringView& name)
diff --git a/Applications/Spreadsheet/Spreadsheet.h b/Applications/Spreadsheet/Spreadsheet.h
index 23fb45d4b1..ab9ecb62d8 100644
--- a/Applications/Spreadsheet/Spreadsheet.h
+++ b/Applications/Spreadsheet/Spreadsheet.h
@@ -118,7 +118,11 @@ public:
     void update();
     void update(Cell&);
 
-    JS::Value evaluate(const StringView&, Cell* = nullptr);
+    struct ValueAndException {
+        JS::Value value;
+        JS::Exception* exception { nullptr };
+    };
+    ValueAndException evaluate(const StringView&, Cell* = nullptr);
     JS::Interpreter& interpreter() const;
     SheetGlobalObject& global_object() const { return *m_global_object; }
 
diff --git a/Applications/Spreadsheet/SpreadsheetModel.cpp b/Applications/Spreadsheet/SpreadsheetModel.cpp
index 2c3826801e..84b34a5e99 100644
--- a/Applications/Spreadsheet/SpreadsheetModel.cpp
+++ b/Applications/Spreadsheet/SpreadsheetModel.cpp
@@ -37,16 +37,6 @@ SheetModel::~SheetModel()
 {
 }
 
-static inline JS::Object* as_error(JS::Value value)
-{
-    if (value.is_object()) {
-        auto& object = value.as_object();
-        return object.is_error() ? &object : nullptr;
-    }
-
-    return nullptr;
-}
-
 GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role) const
 {
     if (!index.is_valid())
@@ -58,10 +48,22 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role)
             return String::empty();
 
         if (cell->kind() == Spreadsheet::Cell::Formula) {
-            if (auto object = as_error(cell->evaluated_data())) {
+            if (auto exception = cell->exception()) {
                 StringBuilder builder;
-                auto error = object->get("message").to_string_without_side_effects();
                 builder.append("Error: ");
+                auto value = exception->value();
+                if (value.is_object()) {
+                    auto& object = value.as_object();
+                    if (object.is_error()) {
+                        auto error = object.get("message").to_string_without_side_effects();
+                        builder.append(error);
+                        return builder.to_string();
+                    }
+                }
+                auto error = value.to_string(cell->sheet().global_object());
+                // This is annoying, but whatever.
+                cell->sheet().interpreter().vm().clear_exception();
+
                 builder.append(error);
                 return builder.to_string();
             }
@@ -87,7 +89,7 @@ GUI::Variant SheetModel::data(const GUI::ModelIndex& index, GUI::ModelRole role)
             return {};
 
         if (cell->kind() == Spreadsheet::Cell::Formula) {
-            if (as_error(cell->evaluated_data()))
+            if (cell->exception())
                 return Color(Color::Red);
         }
 
diff --git a/Libraries/LibJS/AST.cpp b/Libraries/LibJS/AST.cpp
index 154c3a2719..563a278f84 100644
--- a/Libraries/LibJS/AST.cpp
+++ b/Libraries/LibJS/AST.cpp
@@ -51,7 +51,7 @@
 
 namespace JS {
 
-static void update_function_name(Value value, const FlyString& name, HashTable<const JS::Cell*>& visited)
+static void update_function_name(Value value, const FlyString& name, HashTable<JS::Cell*>& visited)
 {
     if (!value.is_object())
         return;
@@ -73,7 +73,7 @@ static void update_function_name(Value value, const FlyString& name, HashTable<c
 
 static void update_function_name(Value value, const FlyString& name)
 {
-    HashTable<const JS::Cell*> visited;
+    HashTable<JS::Cell*> visited;
     update_function_name(value, name, visited);
 }
 
diff --git a/Libraries/LibJS/Heap/Handle.h b/Libraries/LibJS/Heap/Handle.h
index e2686395fc..33fb022d21 100644
--- a/Libraries/LibJS/Heap/Handle.h
+++ b/Libraries/LibJS/Heap/Handle.h
@@ -65,6 +65,8 @@ public:
     T* cell() { return static_cast<T*>(m_impl->cell()); }
     const T* cell() const { return static_cast<const T*>(m_impl->cell()); }
 
+    bool is_null() const { return m_impl.is_null(); }
+
 private:
     explicit Handle(NonnullRefPtr<HandleImpl> impl)
         : m_impl(move(impl))