Add finalize_cb to jerry_context_data_manager_t #2269

martijnthe · 2018-04-04T09:05:39Z

This patch adds a new finalize_cb callback to jerry_context_data_manager_t.
The callback is run as the very last thing in jerry_cleanup, after the VM has been torn down entirely.
There was already the deinit_cb, which is run while the VM is still in the process of being torn down.
The reason the deinit_cb is not always sufficient is that there may still be objects alive (because they still being referenced) that have native pointers associated with the context manager that is being deinit'ed.
As a result, the free_cbs for those objects can get called after the associated context manager's deinit_cb is run. This makes cleanup of manager state that is depended on by the live objects impossible to do in the deinit_cb. That type of cleanup can only be done when all values have been torn down completely.

JerryScript-DCO-1.0-Signed-off-by: Martijn The martijn.the@intel.com

martijnthe · 2018-04-04T09:06:32Z

@gabrielschulhof thoughts / comments?

@zherczeg I will add tests + docs once the general idea, naming, etc. have been agreed upon.

martijnthe · 2018-04-04T09:39:51Z

jerry-core/api/jerry.c

    if (this_p->manager_p->deinit_cb)
    {
      this_p->manager_p->deinit_cb (JERRY_CONTEXT_DATA_HEADER_USER_DATA (this_p));
    }
-    jmem_heap_free_block (this_p, sizeof (jerry_context_data_header_t) + this_p->manager_p->bytes_needed);
  }

  ecma_finalize ();
  jmem_finalize ();
  jerry_make_api_unavailable ();


Oh shoot I need to move jmem_finalize () to the end, because jmem_heap_free_block is used below.
Is that a problem?

Don't think so.

zherczeg · 2018-04-04T12:37:31Z

The name is ok for me.

martijnthe · 2018-04-04T13:03:02Z

The name is ok for me.

OK, thanks.

I extended the unit tests + updated the docs.

gabrielschulhof

LGTM

zherczeg · 2018-04-05T08:47:59Z

Patch is good. But I have a generic question: since the functionality is changed for deinit_cb, does this affect existing jerry extensions?

martijnthe · 2018-04-05T08:58:35Z

But I have a generic question: since the functionality is changed for deinit_cb, does this affect existing jerry extensions?

@zherczeg Which change are you referring to? That the deallocation of the buffer happens a little bit later? I don't think it affects existing jerry extensions.

This patch adds a new finalize_cb callback to jerry_context_data_manager_t. The callback is run as the very last thing in jerry_cleanup, after the VM has been torn down entirely. There was already the deinit_cb, which is run while the VM is still in the process of being torn down. The reason the deinit_cb is not always sufficient is that there may still be objects alive (because they still being referenced) that have native pointers associated with the context manager that is being deinit'ed. As a result, the free_cb's for those objects can get called *after* the associated context manager's deinit_cb is run. This makes cleanup of manager state that is depended on by the live objects impossible to do in the deinit_cb. That type of cleanup can only be done when all values have been torn down completely. JerryScript-DCO-1.0-Signed-off-by: Martijn The martijn.the@intel.com

martijnthe · 2018-04-05T09:13:51Z

OK, I rebased it.

zherczeg · 2018-04-05T09:19:12Z

Actually it happens earlier if I understand your patch. But ok, if they are not affected.

martijnthe · 2018-04-05T09:25:49Z

Yeah, no they are not affected.

zherczeg

LGTM

LaszloLango

LGTM

* Add the ability to throw an error to python debugger (jerryscript-project#2188) This patch makes it possible to throw an error from the python debugger client using the `throw` command. JerryScript-DCO-1.0-Signed-off-by: Daniel Balla dballa@inf.u-szeged.hu * Fix typo and redundant text in the documentation. (jerryscript-project#2260) JerryScript-DCO-1.0-Signed-off-by: Daniel Vince vinced@inf.u-szeged.hu * Fix accessing the contents of a direct string (jerryscript-project#2261) In the `ecma_string_get_chars` method the contents of a direct string is accessed incorrectly. It tries to extract the magic string id from the string pointer but the direct string does not need this step. JerryScript-DCO-1.0-Signed-off-by: Peter Gal pgal.u-szeged@partner.samsung.com * Remove websocket message macros in debugger (jerryscript-project#2262) JERRY_DEBUGGER_INIT_SEND_MESSAGE JERRY_DEBUGGER_SET_SEND_MESSAGE_SIZE JERRY_DEBUGGER_SET_SEND_MESSAGE_SIZE_FROM_TYPE JerryScript-DCO-1.0-Signed-off-by: Jimmy Huang jimmy.huang@intel.com * Remove TARGET_HOST macros (jerryscript-project#2264) Remove TARGET_HOST defines from the jerry-libc module and replace with compiler provided macros. JerryScript-DCO-1.0-Signed-off-by: Istvan Miklos imiklos2@inf.u-szeged.hu * Fix bug in stringToCesu8 conversion for 0x7ff (jerryscript-project#2267) JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com * Add ecma_free_all_enqueued_jobs function (jerryscript-project#2265) This function releases any remaining promise job that wasn't completed. Also added this function to `jerry_cleanup ()`, therefore it will be automatically run. JerryScript-DCO-1.0-Signed-off-by: Daniel Balla dballa@inf.u-szeged.hu * Improve jerry_is_feature_enabled with object availability information (jerryscript-project#2250) JerryScript-DCO-1.0-Signed-off-by: Zsolt Raduska rzsolt@inf.u-szeged.hu * Add json parse and stringify function to jerryscript c api (jerryscript-project#2243) JerryScript-DCO-1.0-Signed-off-by: Zsolt Raduska rzsolt@inf.u-szeged.hu * Simplify debugger-ws.h (jerryscript-project#2266) Remove several macros and types from it. This change simplifies the external debugger interface. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com * Add finalize_cb to jerry_context_data_manager_t (jerryscript-project#2269) This patch adds a new finalize_cb callback to jerry_context_data_manager_t. The callback is run as the very last thing in jerry_cleanup, after the VM has been torn down entirely. There was already the deinit_cb, which is run while the VM is still in the process of being torn down. The reason the deinit_cb is not always sufficient is that there may still be objects alive (because they still being referenced) that have native pointers associated with the context manager that is being deinit'ed. As a result, the free_cb's for those objects can get called *after* the associated context manager's deinit_cb is run. This makes cleanup of manager state that is depended on by the live objects impossible to do in the deinit_cb. That type of cleanup can only be done when all values have been torn down completely. JerryScript-DCO-1.0-Signed-off-by: Martijn The martijn.the@intel.com * Rework snapshot generation API. (jerryscript-project#2259) Also remove eval context support. It provides no practical advantage. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com * Implement the ES2015 version of Object.getPrototypeOf and add a test file for it (jerryscript-project#2256) JerryScript-DCO-1.0-Signed-off-by: Peter Marki marpeter@inf.u-szeged.hu * Move DevTools integration code into jerry-client-ts subdir JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com * Fix some things to match the new directory for TS code JerryScript-DCO-1.0-Signed-off-by: Geoff Gustafson geoff@linux.intel.com

…2269) This patch adds a new finalize_cb callback to jerry_context_data_manager_t. The callback is run as the very last thing in jerry_cleanup, after the VM has been torn down entirely. There was already the deinit_cb, which is run while the VM is still in the process of being torn down. The reason the deinit_cb is not always sufficient is that there may still be objects alive (because they still being referenced) that have native pointers associated with the context manager that is being deinit'ed. As a result, the free_cb's for those objects can get called *after* the associated context manager's deinit_cb is run. This makes cleanup of manager state that is depended on by the live objects impossible to do in the deinit_cb. That type of cleanup can only be done when all values have been torn down completely. JerryScript-DCO-1.0-Signed-off-by: Martijn The martijn.the@intel.com

…ment This patch is the proper fix for jerryscript-project#2667, since jerryscript-project#2269 did not fix the problem entirely. JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

This patch is the proper fix for jerryscript-project#2667, since jerryscript-project#2269 did not fix the problem entirely. JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

This patch is the proper fix for #2667, since #2269 did not fix the problem entirely. JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik frobert@inf.u-szeged.hu

martijnthe commented Apr 4, 2018

View reviewed changes

martijnthe force-pushed the context-manager-finalize-cb branch from 11949c8 to 53095c8 Compare April 4, 2018 13:02

martijnthe force-pushed the context-manager-finalize-cb branch from 53095c8 to 10cdc5d Compare April 4, 2018 13:57

gabrielschulhof approved these changes Apr 4, 2018

View reviewed changes

martijnthe force-pushed the context-manager-finalize-cb branch from 10cdc5d to 9c36dfb Compare April 5, 2018 09:04

zherczeg approved these changes Apr 5, 2018

View reviewed changes

LaszloLango approved these changes Apr 5, 2018

View reviewed changes

LaszloLango merged commit 35926f3 into jerryscript-project:master Apr 5, 2018

rerobika mentioned this pull request Jan 9, 2019

Inner classes should not finalize the parent's class heritage environment #2686

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add finalize_cb to jerry_context_data_manager_t #2269

Add finalize_cb to jerry_context_data_manager_t #2269

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

martijnthe Apr 4, 2018

Uh oh!

zherczeg Apr 4, 2018

Uh oh!

zherczeg commented Apr 4, 2018

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

gabrielschulhof left a comment

Uh oh!

zherczeg commented Apr 5, 2018

Uh oh!

martijnthe commented Apr 5, 2018

Uh oh!

martijnthe commented Apr 5, 2018

Uh oh!

zherczeg commented Apr 5, 2018

Uh oh!

martijnthe commented Apr 5, 2018

Uh oh!

zherczeg left a comment

Uh oh!

LaszloLango left a comment

Uh oh!

Uh oh!

Add finalize_cb to jerry_context_data_manager_t #2269

Add finalize_cb to jerry_context_data_manager_t #2269

Uh oh!

Conversation

martijnthe commented Apr 4, 2018

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

martijnthe Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

zherczeg Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Apr 4, 2018

Uh oh!

martijnthe commented Apr 4, 2018

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Apr 5, 2018

Uh oh!

martijnthe commented Apr 5, 2018

Uh oh!

martijnthe commented Apr 5, 2018

Uh oh!

zherczeg commented Apr 5, 2018

Uh oh!

martijnthe commented Apr 5, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!