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.

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;
 }
 
 }

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;

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
 }
 

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());
 }
 

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) {

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;
 };
 

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)

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; }
 

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);
         }
 

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);
 }
 

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))