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.

Sign up for GitHub

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

Jump to bottom

inspector: zero out structure members #8536

Closed
wants to merge 1 commit into from
Closed

inspector: zero out structure members #8536

wants to merge 1 commit into from

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Sep 14, 2016

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

Change is internal to inspector.

Description of change

Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.

This is an attempt at guessing a fix for #8155 - so far I was not able to repro it.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Sep 14, 2016
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 14, 2016

@ofrobots

@ofrobots ofrobots mentioned this pull request Sep 14, 2016
Closed
@ofrobots
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/4053/.

bnoordhuis
bnoordhuis approved these changes Sep 15, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe make the structs DISALLOW_COPY_AND_ASSIGN while you're here.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 15, 2016

I've added DISALLOW_COPY_AND_ASSIGN. Then it showed that inspector socket instance needs a cleanup to be reused - so I decided that it would be best if inspector_accept did a reset, instead of expecting a clean instance.

Please take another look.

@ofrobots
Copy link
Contributor

New CI: https://ci.nodejs.org/job/node-test-pull-request/4057/

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 15, 2016

Sorry, forgot to run the linter. I uploaded a fixed version.

@ofrobots
Copy link
Contributor

https://ci.nodejs.org/job/node-test-pull-request/4058/

bnoordhuis
bnoordhuis requested changes Sep 16, 2016
View reviewed changes
src/inspector_socket.cc
inspector->buffer.clear();
inspector->ws_mode = false;
inspector->shutting_down = false;
inspector->connection_eof = false;
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it should be a inspector_socket_s method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. I introduced the function... But that pushed the struct to class territory too far for me :)

I changed the struct to class, renamed it appropriately and added the namespace. In fairness, other inspector_* functions need to become member functions as well and the state needs to be made private, etc.

But I am currently working on a complex refactoring that will significantly change this class (I'm hoping to remove libuv interactions) so I don't see much value in doing a comprehensive change right now...

Please take a look at updated CL.

bnoordhuis
bnoordhuis requested changes Sep 19, 2016
View reviewed changes
src/inspector_socket.cc
inspector_socket_t* inspector =
reinterpret_cast<inspector_socket_t*>(parser->data);
InspectorSocket* inspector =
reinterpret_cast<InspectorSocket*>(parser->data);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe switch to static_cast while here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/inspector_socket.h
public:
InspectorSocket() : data(nullptr), http_parsing_state(nullptr),
ws_state(nullptr), buffer(0), ws_mode(false),
shutting_down(false), connection_eof(false) { }
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is off here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/inspector_socket.h
handshake_cb callback);

void inspector_close(struct inspector_socket_s* inspector,
void inspector_close(struct InspectorSocket* inspector,
Copy link
Member

Choose a reason for hiding this comment

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

Can you drop the struct keywords?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

test/cctest/test_inspector_socket.cc
@@ -95,7 +97,7 @@ static bool connected_cb(inspector_socket_t* socket,
static void on_new_connection(uv_stream_t* server, int status) {
GTEST_ASSERT_EQ(0, status);
connected = true;
inspector_accept(server, reinterpret_cast<inspector_socket_t*>(server->data),
inspector_accept(server, reinterpret_cast<InspectorSocket*>(server->data),
Copy link
Member

Choose a reason for hiding this comment

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

static_cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 19, 2016

Thank you for the review. I updated the code, please take another look.

bnoordhuis
bnoordhuis approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/4144/

inspector: zero out structure members
39b9d13 
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.

Attemp at fixing #8155 (so far I was not able to repro it)
@ofrobots
Copy link
Contributor

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/4159/

@ofrobots
Copy link
Contributor

Thanks, landed as bc1ebd6.

@ofrobots ofrobots closed this Sep 20, 2016
ofrobots pushed a commit that referenced this pull request Sep 20, 2016
@ofrobots
inspector: zero out structure members
bc1ebd6 
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.

Attempt at fixing #8155 (so far I was not able to repro it)

PR-URL: #8536
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@ofrobots ofrobots added the v6.x label Sep 28, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
inspector: zero out structure members
06e782f 
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.

Attempt at fixing nodejs#8155 (so far I was not able to repro it)

PR-URL: nodejs#8536
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
inspector: zero out structure members
b05ce84 
Ctor has to be added as memset to 0 is no longer an option, since
the structure now has std::vector member.

Attempt at fixing #8155 (so far I was not able to repro it)

PR-URL: #8536
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@eugeneo eugeneo deleted the zero_out branch October 13, 2016 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eugeneo @ofrobots @bnoordhuis @MylesBorins @nodejs-github-bot