Skip to content

Commit

Permalink
SpreadSheet: Fix that the js integration abused global objects
Browse files Browse the repository at this point in the history
Before this commit it only allocated the global object so when it wanted
to lookup 'thisSheet' it could not find it in the global environment.
We now hotswap the global object everytime a cell evaluated.

This also fixes that SheetGlobalObject did not have an
internal_has_property meaning 'A0' could not be referenced unless it was
via a member lookup (this.A0). This was already broken before the
bindings refactoring.

The correct behavior of realms in spreadsheet is not completely clear
since what is shared between sheets is not very well defined.

The reason that just setting the SheetGlobalObject as the
global_this_value is not enough is because ECMAScript does not check the
global_this_value for members when resolving a reference in the global
environment.
  • Loading branch information
davidot authored and linusg committed Sep 30, 2021
1 parent e5d48ee commit 5611285
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 1 deletion.
12 changes: 11 additions & 1 deletion Userland/Applications/Spreadsheet/JSIntegration.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include "Spreadsheet.h"
#include "Workbook.h"
#include <LibJS/Lexer.h>
#include <LibJS/Runtime/Completion.h>
#include <LibJS/Runtime/Error.h>
#include <LibJS/Runtime/GlobalObject.h>
#include <LibJS/Runtime/Object.h>
Expand Down Expand Up @@ -102,6 +101,17 @@ SheetGlobalObject::~SheetGlobalObject()
{
}

JS::ThrowCompletionOr<bool> SheetGlobalObject::internal_has_property(JS::PropertyName const& name) const
{
if (name.is_string()) {
if (name.as_string() == "value")
return true;
if (m_sheet.parse_cell_name(name.as_string()).has_value())
return true;
}
return Object::internal_has_property(name);
}

JS::ThrowCompletionOr<JS::Value> SheetGlobalObject::internal_get(const JS::PropertyName& property_name, JS::Value receiver) const
{
if (property_name.is_string()) {
Expand Down
1 change: 1 addition & 0 deletions Userland/Applications/Spreadsheet/JSIntegration.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class SheetGlobalObject final : public JS::GlobalObject {

virtual ~SheetGlobalObject() override;

virtual JS::ThrowCompletionOr<bool> internal_has_property(JS::PropertyName const& name) const override;
virtual JS::ThrowCompletionOr<JS::Value> internal_get(JS::PropertyName const&, JS::Value receiver) const override;
virtual JS::ThrowCompletionOr<bool> internal_set(JS::PropertyName const&, JS::Value value, JS::Value receiver) override;
virtual void initialize_global_object() override;
Expand Down
3 changes: 3 additions & 0 deletions Userland/Applications/Spreadsheet/Spreadsheet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ Sheet::ValueAndException Sheet::evaluate(const StringView& source, Cell* on_beha
if (parser.has_errors() || interpreter().exception())
return { JS::js_undefined(), interpreter().exception() };

// FIXME: This creates a GlobalEnvironment for every evaluate call which we might be able to circumvent with multiple realms.
interpreter().realm().set_global_object(global_object(), &global_object());

interpreter().run(global_object(), program);
if (interpreter().exception()) {
auto exc = interpreter().exception();
Expand Down

0 comments on commit 5611285

Please sign in to comment.