{"action":"created","issue":{"url":"https://api.github.com/repos/jerryscript-project/jerryscript/issues/2518","repository_url":"https://api.github.com/repos/jerryscript-project/jerryscript","labels_url":"https://api.github.com/repos/jerryscript-project/jerryscript/issues/2518/labels{/name}","comments_url":"https://api.github.com/repos/jerryscript-project/jerryscript/issues/2518/comments","events_url":"https://api.github.com/repos/jerryscript-project/jerryscript/issues/2518/events","html_url":"https://github.com/jerryscript-project/jerryscript/pull/2518","id":359053914,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE0NjM3Mzg1","number":2518,"title":"Fix an issue with ecma collections in the Promise implementation","user":{"login":"DanielBallaSZTE","id":20175069,"node_id":"MDQ6VXNlcjIwMTc1MDY5","avatar_url":"https://avatars0.githubusercontent.com/u/20175069?v=4","gravatar_id":"","url":"https://api.github.com/users/DanielBallaSZTE","html_url":"https://github.com/DanielBallaSZTE","followers_url":"https://api.github.com/users/DanielBallaSZTE/followers","following_url":"https://api.github.com/users/DanielBallaSZTE/following{/other_user}","gists_url":"https://api.github.com/users/DanielBallaSZTE/gists{/gist_id}","starred_url":"https://api.github.com/users/DanielBallaSZTE/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DanielBallaSZTE/subscriptions","organizations_url":"https://api.github.com/users/DanielBallaSZTE/orgs","repos_url":"https://api.github.com/users/DanielBallaSZTE/repos","events_url":"https://api.github.com/users/DanielBallaSZTE/events{/privacy}","received_events_url":"https://api.github.com/users/DanielBallaSZTE/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-09-11T13:43:05Z","updated_at":"2018-09-12T17:16:12Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/jerryscript-project/jerryscript/pulls/2518","html_url":"https://github.com/jerryscript-project/jerryscript/pull/2518","diff_url":"https://github.com/jerryscript-project/jerryscript/pull/2518.diff","patch_url":"https://github.com/jerryscript-project/jerryscript/pull/2518.patch"},"body":"Fixes #2486\r\nFixes #2506\r\n\r\nCo-authored-by: Robert Fancsik frobert@inf.u-szeged.hu\r\nJerryScript-DCO-1.0-Signed-off-by: Daniel Balla dballa@inf.u-szeged.hu"},"comment":{"url":"https://api.github.com/repos/jerryscript-project/jerryscript/issues/comments/420727677","html_url":"https://github.com/jerryscript-project/jerryscript/pull/2518#issuecomment-420727677","issue_url":"https://api.github.com/repos/jerryscript-project/jerryscript/issues/2518","id":420727677,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMDcyNzY3Nw==","user":{"login":"akosthekiss","id":9976482,"node_id":"MDQ6VXNlcjk5NzY0ODI=","avatar_url":"https://avatars0.githubusercontent.com/u/9976482?v=4","gravatar_id":"","url":"https://api.github.com/users/akosthekiss","html_url":"https://github.com/akosthekiss","followers_url":"https://api.github.com/users/akosthekiss/followers","following_url":"https://api.github.com/users/akosthekiss/following{/other_user}","gists_url":"https://api.github.com/users/akosthekiss/gists{/gist_id}","starred_url":"https://api.github.com/users/akosthekiss/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/akosthekiss/subscriptions","organizations_url":"https://api.github.com/users/akosthekiss/orgs","repos_url":"https://api.github.com/users/akosthekiss/repos","events_url":"https://api.github.com/users/akosthekiss/events{/privacy}","received_events_url":"https://api.github.com/users/akosthekiss/received_events","type":"User","site_admin":false},"created_at":"2018-09-12T17:16:12Z","updated_at":"2018-09-12T17:16:12Z","author_association":"MEMBER","body":"Analysis is coming (this gona be long).\r\n\r\nSo, first on the variant when `ecma_reject_promise` **is changed**. Here is what Travis CI is reporting, if the \"GC can be triggered by ecma_new_values_collection so freeing the collection first and creating a new one might cause a heap after use event\" part in `ecma_reject_promise` is tampered with:\r\n\r\n```\r\n[  58/ 395]FAIL (1): tests/jerry/es2015/regression-test-issue-1881.js\r\n\r\n=================================================================\r\n==16738==ERROR: AddressSanitizer: heap-use-after-free on address 0xf4d00698 at pc 0x08050b1d bp 0xffce4ba8 sp 0xffce4b9c\r\nREAD of size 4 at 0xf4d00698 thread T0\r\n    #0 0x8050b1c in ecma_collection_iterator_init /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/base/ecma-helpers-values-collection.c:163\r\n    #1 0x8050fd6 in ecma_gc_mark_promise_object /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/base/ecma-gc.c:230\r\n    #2 0x8050fd6 in ecma_gc_mark /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/base/ecma-gc.c:339\r\n    #3 0x805a823 in ecma_gc_run /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/base/ecma-gc.c:863\r\n    #4 0x807b6cd in jmem_run_free_unused_memory_callbacks /home/travis/build/jerryscript-project/jerryscript/jerry-core/jmem/jmem-allocator.c:267\r\n    #5 0x807b766 in jmem_heap_gc_and_alloc_block.lto_priv.56 /home/travis/build/jerryscript-project/jerryscript/jerry-core/jmem/jmem-heap.c:317\r\n    #6 0x805177b in jmem_heap_alloc_block /home/travis/build/jerryscript-project/jerryscript/jerry-core/jmem/jmem-heap.c:366\r\n    #7 0x805177b in jmem_pools_alloc /home/travis/build/jerryscript-project/jerryscript/jerry-core/jmem/jmem-poolman.c:104\r\n    #8 0x805177b in ecma_new_values_collection /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/base/ecma-helpers-values-collection.c:43\r\n    #9 0x808263c in ecma_reject_promise /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-promise-object.c:183\r\n    #10 0x8082bf0 in ecma_promise_reject_handler /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-promise-object.c:251\r\n    #11 0x80727b2 in ecma_op_function_call /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-function-object.c:589\r\n    #12 0x807361e in ecma_process_promise_reaction_job /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-jobqueue.c:203\r\n    #13 0x805c7d5 in ecma_process_all_enqueued_jobs /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-jobqueue.c:343\r\n    #14 0x805c7d5 in jerry_run_all_enqueued_jobs /home/travis/build/jerryscript-project/jerryscript/jerry-core/api/jerry.c:571\r\n    #15 0x804a5d8 in main /home/travis/build/jerryscript-project/jerryscript/jerry-main/main-unix.c:863\r\n    #16 0xf7007ad2 in __libc_start_main (/lib32/libc.so.6+0x19ad2)\r\n    #17 0x804ad19  (/home/travis/build/jerryscript-project/jerryscript/build/tests/jerry_tests-es2015_subset-debug/local/bin/jerry+0x804ad19)\r\n\r\n0xf4d00698 is located 8 bytes inside of 16-byte region [0xf4d00690,0xf4d006a0)\r\nfreed by thread T0 here:\r\n    #0 0xf724c14e in free (/usr/lib32/libasan.so.2+0x9214e)\r\n    #1 0x8052f7b in jmem_heap_free_block /home/travis/build/jerryscript-project/jerryscript/jerry-core/jmem/jmem-heap.c:490\r\n    #2 0x8052f7b in ecma_free_values_collection /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/base/ecma-helpers-values-collection.c:62\r\n    #3 0x808260f in ecma_reject_promise /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-promise-object.c:180\r\n    #4 0x8082bf0 in ecma_promise_reject_handler /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-promise-object.c:251\r\n    #5 0x80727b2 in ecma_op_function_call /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-function-object.c:589\r\n    #6 0x807361e in ecma_process_promise_reaction_job /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-jobqueue.c:203\r\n    #7 0x805c7d5 in ecma_process_all_enqueued_jobs /home/travis/build/jerryscript-project/jerryscript/jerry-core/ecma/operations/ecma-jobqueue.c:343\r\n    #8 0x805c7d5 in jerry_run_all_enqueued_jobs /home/travis/build/jerryscript-project/jerryscript/jerry-core/api/jerry.c:571\r\n    #9 0x804a5d8 in main /home/travis/build/jerryscript-project/jerryscript/jerry-main/main-unix.c:863\r\n    #10 0xf7007ad2 in __libc_start_main (/lib32/libc.so.6+0x19ad2)\r\n```\r\n\r\nThese logs show that `ecma_reject_promise` calls `ecma_free_values_collection` (freed#3 to freed#2), and then calls `ecma_new_values_collection` (read#9 to read#8), which triggers the GC (read#5 and up), which tries to access the previously freed collection (in read#0). So, it seems to me that the warning comment on why not to rewrite this part of the code is quite accurate and right. Thus, I'd still suggest not to touch this code.\r\n\r\nOn the failure when `ecma_reject_promise` **is not changed**. The logs are then (in a somewhat shortened form):\r\n```\r\n/home/travis/build/jerryscript-project/jerryscript/tools/build.py \\\r\n--profile=es2015-subset \\\r\n--debug \\\r\n\r\n[  70/ 396]FAIL (120): tests/jerry/es2015/regression-test-issue-2486.js\r\n```\r\nAnd this is something that I could reproduce out of the box by\r\n```sh\r\n$ git fetch origin pull/2518/head:pr2518\r\n$ git checkout pr2518\r\n$ tools/build.py --profile=es2015-subset --debug\r\n$ build/bin/jerry tests/jerry/es2015/regression-test-issue-2486.js\r\n$ echo $?\r\n120\r\n```\r\nWith a slightly enhanced command line, the ICE is also printed:\r\n```sh\r\n$ tools/build.py --profile=es2015-subset --debug --logging=on\r\n$ build/bin/jerry tests/jerry/es2015/regression-test-issue-2486.js\r\nICE: Assertion 'ecma_is_value_boolean (handler) || ecma_op_is_callable (handler)' failed at jerry-core/ecma/operations/ecma-jobqueue.c(ecma_process_promise_reaction_job):158.\r\nError: ERR_FAILED_INTERNAL_ASSERTION\r\n$ echo $?\r\n120\r\n```\r\nThis is failure 3 from the issue report. This tells me that this PR is not fixing issue 2486.\r\n\r\nAnd one more thing, on the rest of the PR. If I use the above build, the other new test (from issue 2506) is just failing fine (i.e., without an assertion error):\r\n```sh\r\n$ build/bin/jerry tests/jerry/fail/regression-test-issue-2506.js \r\nfalse\r\nScript Error: ReferenceError\r\n$ echo $?\r\n1\r\n```\r\nHowever, if I build jerry as reported in the issue (i.e., with error messages enabled), then the ICE is still there:\r\n```sh\r\n$ tools/build.py --clean --debug --error-messages=on --profile=es2015-subset --logging=on\r\n$ build/bin/jerry tests/jerry/fail/regression-test-issue-2506.js \r\nfalse\r\nScript Error: ReferenceError: $ is not defined\r\nICE: Assertion 'ECMA_PROPERTY_IS_PROPERTY_PAIR (prop_iter_p)' failed at jerry-core/ecma/base/ecma-helpers.c(ecma_find_named_property):629.\r\nError: ERR_FAILED_INTERNAL_ASSERTION\r\n$ echo $?\r\n120\r\n```\r\nThis is the error from the issue report. This tells me that this PR is not fixing 2506 either, unfortunately, and the proposed code changes are unnecessary in their current form.\r\n\r\n(Unfortunately, this may also hint at the test framework not being able to catch issue 2506 with its current build settings.)"}}
{"actor":{"display_login":"akosthekiss"}}