Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: remove debugger dead code #12621

Closed
wants to merge 2 commits into from

Conversation

@targos
Copy link
Member

commented Apr 24, 2017

/cc @bnoordhuis

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

debugger

@cjihrig
Copy link
Contributor

left a comment

LGTM if the CI passes

@bnoordhuis
Copy link
Member

left a comment

Thanks, LGTM.

@targos

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

@targos

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

It seems like this is causing a problem on 32-bit platforms

@@ -25,6 +25,7 @@
#include "node_crypto.h"
#include "node_crypto_bio.h"
#include "node_crypto_groups.h"
#include "node_mutex.h"

This comment has been minimized.

Copy link
@refack

refack Apr 25, 2017

Member

How is this related?

This comment has been minimized.

Copy link
@targos

targos Apr 25, 2017

Author Member

The file needs this #include. It was working because debug-agent.h had it.

This comment has been minimized.

Copy link
@refack
@refack

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

On 32 posix cctest:EnvironmentTest.AtExitWithArgument segfaults.
On x86 windows it passes

[ RUN      ] EnvironmentTest.AtExitWithArgument
make[1]: *** [test-ci] Segmentation fault (core dumped)

Maybe you left an uninitialized variable that when exiting trying to free it segfaults?

@refack
refack approved these changes Apr 25, 2017
@@ -25,6 +25,7 @@
#include "node_crypto.h"
#include "node_crypto_bio.h"
#include "node_crypto_groups.h"
#include "node_mutex.h"

This comment has been minimized.

Copy link
@refack
@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

[expand for backtrace & register info]
(gdb) bt
#0  0x09024148 in v8::platform::DefaultPlatform::~DefaultPlatform() ()
#1  0x090243d0 in v8::platform::DefaultPlatform::~DefaultPlatform() ()
#2  0x08f7ea16 in EnvironmentTest::TearDown() ()
#3  0x08feb3fd in testing::Test::Run() [clone .part.450] ()
#4  0x08feb6b4 in testing::TestInfo::Run() [clone .part.451] ()
#5  0x08feb885 in testing::TestCase::Run() [clone .part.452] ()
#6  0x08fec5e3 in testing::internal::UnitTestImpl::RunAllTests() [clone .part.455] ()
#7  0x08fec89c in testing::UnitTest::Run() ()
#8  0x0843ef57 in main ()
(gdb) info registers 
eax            0x9beacb0	163490992
ecx            0xf7581000	-145223680
edx            0x0	0
ebx            0x9bead38	163491128
esp            0xffdbd260	0xffdbd260
ebp            0xffdbd2a8	0xffdbd2a8
esi            0x9c38ad8	163810008
edi            0x9beacb0	163490992
eip            0x9024148	0x9024148 <v8::platform::DefaultPlatform::~DefaultPlatform()+216>
eflags         0x210206	[ PF IF RF ID ]
cs             0x23	35
ss             0x2b	43
ds             0x2b	43
es             0x2b	43
fs             0x0	0
gs             0x63	99
(gdb) disassemble $eip-40,$eip+40
Dump of assembler code from 0x9024120 to 0x9024170:
   0x09024120 <_ZN2v88platform15DefaultPlatformD2Ev+176>:	cmp    %edx,%ebx
   0x09024122 <_ZN2v88platform15DefaultPlatformD2Ev+178>:	jne    0x9024108 <_ZN2v88platform15DefaultPlatformD2Ev+152>
   0x09024124 <_ZN2v88platform15DefaultPlatformD2Ev+180>:	mov    -0x34(%ebp),%eax
   0x09024127 <_ZN2v88platform15DefaultPlatformD2Ev+183>:	mov    0x90(%eax),%esi
   0x0902412d <_ZN2v88platform15DefaultPlatformD2Ev+189>:	lea    0x88(%eax),%ebx
   0x09024133 <_ZN2v88platform15DefaultPlatformD2Ev+195>:	mov    %eax,%edi
   0x09024135 <_ZN2v88platform15DefaultPlatformD2Ev+197>:	cmp    %ebx,%esi
   0x09024137 <_ZN2v88platform15DefaultPlatformD2Ev+199>:	je     0x902418b <_ZN2v88platform15DefaultPlatformD2Ev+283>
   0x09024139 <_ZN2v88platform15DefaultPlatformD2Ev+201>:	lea    0x0(%esi,%eiz,1),%esi
   0x09024140 <_ZN2v88platform15DefaultPlatformD2Ev+208>:	mov    0x1c(%esi),%edx
   0x09024143 <_ZN2v88platform15DefaultPlatformD2Ev+211>:	jmp    0x9024171 <_ZN2v88platform15DefaultPlatformD2Ev+257>
   0x09024145 <_ZN2v88platform15DefaultPlatformD2Ev+213>:	lea    0x0(%esi),%esi
=> 0x09024148 <_ZN2v88platform15DefaultPlatformD2Ev+216>:	mov    (%edx),%eax
   0x0902414a <_ZN2v88platform15DefaultPlatformD2Ev+218>:	test   %eax,%eax
   0x0902414c <_ZN2v88platform15DefaultPlatformD2Ev+220>:	je     0x902415d <_ZN2v88platform15DefaultPlatformD2Ev+237>
   0x0902414e <_ZN2v88platform15DefaultPlatformD2Ev+222>:	mov    (%eax),%edx
   0x09024150 <_ZN2v88platform15DefaultPlatformD2Ev+224>:	sub    $0xc,%esp
   0x09024153 <_ZN2v88platform15DefaultPlatformD2Ev+227>:	push   %eax
   0x09024154 <_ZN2v88platform15DefaultPlatformD2Ev+228>:	call   *0x4(%edx)
   0x09024157 <_ZN2v88platform15DefaultPlatformD2Ev+231>:	mov    0x1c(%esi),%edx
   0x0902415a <_ZN2v88platform15DefaultPlatformD2Ev+234>:	add    $0x10,%esp
   0x0902415d <_ZN2v88platform15DefaultPlatformD2Ev+237>:	mov    0x24(%esi),%eax
   0x09024160 <_ZN2v88platform15DefaultPlatformD2Ev+240>:	sub    $0x4,%eax
   0x09024163 <_ZN2v88platform15DefaultPlatformD2Ev+243>:	cmp    %eax,%edx
   0x09024165 <_ZN2v88platform15DefaultPlatformD2Ev+245>:	je     0x9024350 <_ZN2v88platform15DefaultPlatformD2Ev+736>
   0x0902416b <_ZN2v88platform15DefaultPlatformD2Ev+251>:	add    $0x4,%edx
   0x0902416e <_ZN2v88platform15DefaultPlatformD2Ev+254>:	mov    %edx,0x1c(%esi)

Some printf() debugging hints that the segfault occurs in the loop for tearing down the main_thread_queue_, inside the i->second.front() call (??).

Unfortunately, this only seems to happen in Release mode. and while single-stepping through the destructor in a debugger the segfault did not show, so it might be a race condition… :/ I might have some time to help looking deeper into it later today.

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

Okay, I think I have an idea of what’s going on. The test fixtures create multiple node::Environments that all use the uv_default_loop(), and since the test doesn’t clean up the handles created by Environment::Start(), the default libuv loop structure contains dangling pointers after the first Environment is freed, which then means that creating new handles leads to memory corruption.

This is enough to fix the tests for me:

diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
index a7579ac11671..79bfbf50b45c 100644
--- a/test/cctest/test_environment.cc
+++ b/test/cctest/test_environment.cc
@@ -42,6 +42,7 @@ class EnvironmentTest : public NodeTestFixture {
 
     ~Env() {
       FreeIsolateData(isolate_data_);
+      environment_->CleanupHandles();
       FreeEnvironment(environment_);
     }

But it makes me wonder, should CleanupHandles() be part of FreeEnvironment()? Should it be part of the public API in some other way? @bnoordhuis @danbev

(edit: sorry @targos if this is spoiling the fun of debugging here 😄)

@danbev

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

But it makes me wonder, should CleanupHandles() be part of FreeEnvironment()? Should it be part of the public API in some other way? @bnoordhuis @danbev

Sorry, I missed this issue completely. I was not aware of CleanupHandles() and I'll defer that to @bnoordhuis as I don't know enough yet but that sounds reasonable to me.

@targos

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

But it makes me wonder, should CleanupHandles() be part of FreeEnvironment()?

Or maybe part of the Environment destructor?
BTW Environment::CleanupHandles() is unused by the code base at the moment.

Anyway, thank you for investigating this @addaleax! I've applied your patch for now.
CI: https://ci.nodejs.org/job/node-test-pull-request/7701/

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

Or maybe part of the Environment destructor?

See #12344 for why the answer to that question unfortunately is 'no'.

@refack

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

Windows 10, VS2015x64, one test fails

23	parallel/test-child-process-fork-regr-gh-2847	
duration_ms	
0.313
severity	
fail
stack	
c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-child-process-fork-regr-gh-2847.js:61
            throw err;
            ^

Error: write EMFILE
    at exports._errnoException (util.js:1026:11)
    at ChildProcess.target._send (internal/child_process.js:663:20)
    at ChildProcess.target.send (internal/child_process.js:547:19)
    at Worker.send (internal/cluster/worker.js:54:28)
    at Socket.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-child-process-fork-regr-gh-2847.js:33:14)
    at Object.onceWrapper (events.js:312:19)
    at emitNone (events.js:105:13)
    at Socket.emit (events.js:207:7)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1102:10)

May be a Just a flake , I'm trying to repro localy

@refack

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

@@ -42,6 +42,7 @@ class EnvironmentTest : public NodeTestFixture {

~Env() {
FreeIsolateData(isolate_data_);
environment_->CleanupHandles();

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 27, 2017

Member

I would suggest landing this in a separate commit, it’s a logically not part of the rest of the changes, and we probably want to backport it (e.g. as part of #12664)

src: remove debugger dead code
PR-URL: #12621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@targos targos force-pushed the targos:remove-debugger branch Apr 27, 2017

test: cleanup handles in test_environment
The test fixtures create multiple node::Environments that all use the
uv_default_loop(), and since the test does not clean up the handles
created by Environment::Start(), the default libuv loop structure
contains dangling pointers after the first Environment is freed,
which then means that creating new handles leads to memory corruption.

PR-URL: #12621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

@targos targos force-pushed the targos:remove-debugger branch to 7c8594c Apr 27, 2017

@targos

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2017

@addaleax I kept the test fix as a separate commit and set you as the author since it's your finding and I used your very good explanation for the commit message.
Is it OK to land like this?

@targos

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2017

ping @addaleax

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

@targos Ah, sorry, missed the ping! Yes, this is perfectly fine :)

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

Actually, I went ahead and landed this in d5db4d2, 719247f, I guess that’s okay. Thank you for doing this!

@addaleax addaleax closed this Apr 29, 2017

addaleax added a commit that referenced this pull request Apr 29, 2017
test: cleanup handles in test_environment
The test fixtures create multiple node::Environments that all use the
uv_default_loop(), and since the test does not clean up the handles
created by Environment::Start(), the default libuv loop structure
contains dangling pointers after the first Environment is freed,
which then means that creating new handles leads to memory corruption.

PR-URL: #12621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
addaleax added a commit that referenced this pull request Apr 29, 2017
src: remove debugger dead code
PR-URL: #12621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@targos targos deleted the targos:remove-debugger branch Apr 29, 2017

danbev added a commit to danbev/node that referenced this pull request May 2, 2017
test: cleanup handles in test_environment
The test fixtures create multiple node::Environments that all use the
uv_default_loop(), and since the test does not clean up the handles
created by Environment::Start(), the default libuv loop structure
contains dangling pointers after the first Environment is freed,
which then means that creating new handles leads to memory corruption.

PR-URL: nodejs#12621
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
refack added a commit to refack/node that referenced this pull request May 19, 2017
test: ignore spurious 'EMFILE'
According to the explanation in nodejs#3635#issuecomment-157714683
And as a continuation to nodejs#5422 we also ignore EMFILE
"No more file descriptors are available,so no more files can be opened"

PR-URL: nodejs#12698
Fixes: nodejs#10286
Refs: nodejs#3635 (comment)
Refs: nodejs#5178
Refs: nodejs#5179
Refs: nodejs#4005
Refs: nodejs#5121
Refs: nodejs#5422
Refs: nodejs#12621 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
MylesBorins added a commit that referenced this pull request Jul 17, 2017
test: ignore spurious 'EMFILE'
According to the explanation in #3635#issuecomment-157714683
And as a continuation to #5422 we also ignore EMFILE
"No more file descriptors are available,so no more files can be opened"

PR-URL: #12698
Fixes: #10286
Refs: #3635 (comment)
Refs: #5178
Refs: #5179
Refs: #4005
Refs: #5121
Refs: #5422
Refs: #12621 (comment)
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
hferreiro added a commit to brave/muon that referenced this pull request Sep 11, 2017
hferreiro added a commit to brave/muon that referenced this pull request Sep 20, 2017
hferreiro added a commit to brave/muon that referenced this pull request Dec 27, 2017
hferreiro added a commit to brave/muon that referenced this pull request Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.