test: initialize platform in NodeTestFixture#12469
test: initialize platform in NodeTestFixture#12469sam-github wants to merge 1 commit intonodejs:masterfrom
Conversation
Calling TearDown() without Setup() first is not allowed, but this should fix a coverity warning: *** CID 166971: Uninitialized members (UNINIT_CTOR) /test/cctest/node_test_fixture.h: 97 in NodeTestFixture::NodeTestFixture()() 91 v8::V8::ShutdownPlatform(); 92 delete platform_; 93 platform_ = nullptr; 94 } 95 96 private: >>> CID 166971: Uninitialized members (UNINIT_CTOR) >>> The compiler-generated constructor for this class does not initialize "platform_". 97 v8::Platform* platform_; 98 };
sam-github
force-pushed
the
initialize-platform-in-cctest
branch
from
65e7bd5 to
398cf3a
Compare
| } | ||
|
|
||
| virtual void TearDown() { | ||
| isolate_->Dispose(); |
There was a problem hiding this comment.
@danbev Was the isolate being leaked? Is the dispose needed?
There was a problem hiding this comment.
Yes, it looks like it is currently being leaked. I think dispose is needed or at least I don't think it hurts to have it.
I'm not familiar with Coverity. Is there a way to get this at this information so I can try to clean up any future mess I create? I've looked at https://scan.coverity.com/projects/node-js, but can't really get to much information, I've requested to be added to the project so perhaps once that is approved I might be able to see some more details.
Sorry about this and thanks for fixing it.
|
/to @danbev |
|
github might be slow to update, or maybe there is a bug in status publish: https://ci.nodejs.org/job/node-test-commit/9164/ all platforms passed |
mscdex
added
the
c++
| } | ||
|
|
||
| virtual void TearDown() { | ||
| isolate_->Dispose(); |
There was a problem hiding this comment.
Yes, it looks like it is currently being leaked. I think dispose is needed or at least I don't think it hurts to have it.
I'm not familiar with Coverity. Is there a way to get this at this information so I can try to clean up any future mess I create? I've looked at https://scan.coverity.com/projects/node-js, but can't really get to much information, I've requested to be added to the project so perhaps once that is approved I might be able to see some more details.
Sorry about this and thanks for fixing it.
It may be that scan access is limited to people with visibility into security reports, I'm not sure. Unfortunately, the scans only come after PRs merge, but we don't get that many reports, and its easy to just add them to PR conversations when they do occur. |
|
Already done in #12387 |
8 participants
Calling TearDown() without Setup() first is not allowed, but
this should fix a coverity warning:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test