Skip to content

Conversation

@pmarkee
Copy link
Contributor

@pmarkee pmarkee commented Sep 12, 2018

Fixes #2494

JerryScript-DCO-1.0-Signed-off-by: Peter Marki marpeter@inf.u-szeged.hu

@@ -0,0 +1,2 @@
Object.defineProperty(Object.prototype, "", {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a license header.

JERRY_ASSERT (ecma_is_value_true (put_comp_val));
if (!ecma_is_value_true (put_comp_val))
{
ecma_deref_object (obj_wrapper_p);
Copy link
Member

Choose a reason for hiding this comment

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

Is this enough? Some lines below, the normal code path frees put_comp_val, empty_str_p, and obj_wrapper_p before returning. I think the error handling code path should also do proper cleanup. Perhaps

if (ecma_is_value_true (put_comp_val))
{
  ECMA_TRY_CATCH (str_val,
                  ecma_builtin_json_str (empty_str_p, obj_wrapper_p, &context),
                  ret_value);
  ret_value = ecma_copy_value (str_val);
  ECMA_FINALIZE (str_val);
}
else
{
  ret_value = ECMA_VALUE_UNDEFINED;
}

might work better.

Note: the normal code path mysteriously frees put_comp_val twice. That could be fixed now, I think.

@pmarkee
Copy link
Contributor Author

pmarkee commented Sep 13, 2018

Thanks for the review @akosthekiss , I have updated accordingly.

Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

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

LGTM


if (ecma_is_value_true (put_comp_val))
{
ECMA_TRY_CATCH (str_val,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified. ECMA_TRY_CATCH is unnecessary, the following code is equal:

{
  ret_value = ecma_builtin_json_str (empty_str_p, obj_wrapper_p, &context);
}

Copy link
Member

Choose a reason for hiding this comment

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

is the ecma_copy_value not needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

ecma_copy_value was needed because of the ECMA_FINALIZE. Without the ECMA_TRY_CATCH and ECMA_FINALIZE it is not needed anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. That makes everything a lot simpler.

@akosthekiss akosthekiss added bug Undesired behaviour ecma builtins Related to ECMA built-in routines labels Sep 13, 2018
Fixes #2494

JerryScript-DCO-1.0-Signed-off-by: Peter Marki marpeter@inf.u-szeged.hu
@pmarkee
Copy link
Contributor Author

pmarkee commented Sep 14, 2018

Thanks @LaszloLango , I have updated the PR.

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

@akosthekiss akosthekiss merged commit 0d05dfc into jerryscript-project:master Sep 14, 2018
rerobika added a commit to rerobika/jerryscript that referenced this pull request Jan 2, 2019
…nternal method

This patch fixes jerryscript-project#2652 and fixes jerryscript-project#2653 as well, also reverts jerryscript-project#2521.
Related part of the standard ECMAScript v5.1 15.12.3.10.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu
robertsipka pushed a commit that referenced this pull request Jan 3, 2019
…nternal method (#2665)

This patch fixes #2652 and fixes #2653 as well, also reverts #2521.
Related part of the standard ECMAScript v5.1 15.12.3.10.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu
@pmarkee pmarkee deleted the jsonbug branch February 18, 2019 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Undesired behaviour ecma builtins Related to ECMA built-in routines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants