Skip to content

WS connection should not crash with JS bindings session#17085

Merged
eugeneo merged 1 commit intonodejs:masterfrom
eugeneo:better_socket
Dec 12, 2017
Merged

WS connection should not crash with JS bindings session#17085
eugeneo merged 1 commit intonodejs:masterfrom
eugeneo:better_socket

Conversation

@eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Nov 16, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector, test: reafactor inspector WS socket implementation to be a proper C++

To make it easier to review, I split the change into 2 commits that can be merged after the review.

First commit rewrites WebSockets C API into C++ that allows better management of the object lifecycle. Second implements a change when WS session is not accepted until the session is established on the main thread (this is to ensure there is no current JS bindings session).

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 16, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Nov 16, 2017

Note that I hope this change (the part about clearer object lifetime) will help debugging the intermittent inspector tests on different platforms.

@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Nov 16, 2017
@TimothyGu TimothyGu self-requested a review November 16, 2017 22:49
@eugeneo
Copy link
Contributor Author

eugeneo commented Nov 16, 2017

CI (green): https://ci.nodejs.org/job/node-test-pull-request/11486/

@apapirovski
Copy link
Contributor

apapirovski commented Dec 9, 2017 •
edited
Loading

ping @bnoordhuis, @addaleax & @TimothyGu — could you have a look at this since a review was requested and this PR has been around for a while? Thanks!

@eugeneo would you mind rebasing this if you're still hoping for it to land?

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 11, 2017

@apapirovski I rebased the PR.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 11, 2017

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

inspector: Fix crash for WS connection
73ad3f9 
Attaching WS session will now include a roundtrip onto the main thread
to make sure there is no other session (e.g. JS bindings)

This change also required refactoring WS socket implementation to better
support scenarios like this.

Fixes: #16852
PR-URL: #17085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@eugeneo eugeneo merged commit 73ad3f9 into nodejs:master Dec 12, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 12, 2017

Landed as 73ad3f9

Thank you for the reviews.

@eugeneo eugeneo deleted the better_socket branch December 12, 2017 00:17
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@MylesBorins
inspector: Fix crash for WS connection
fd4529d 
Attaching WS session will now include a roundtrip onto the main thread
to make sure there is no other session (e.g. JS bindings)

This change also required refactoring WS socket implementation to better
support scenarios like this.

Fixes: #16852
PR-URL: #17085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@MylesBorins
inspector: Fix crash for WS connection
54cd7df 
Attaching WS session will now include a roundtrip onto the main thread
to make sure there is no other session (e.g. JS bindings)

This change also required refactoring WS socket implementation to better
support scenarios like this.

Fixes: #16852
PR-URL: #17085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 20, 2017

I suggest we don't land this on 8.x. This issue is rare so I don't think it is justified to apply this change to the old branch.

deepak1556 pushed a commit to electron/electron that referenced this pull request Jan 6, 2018
@codebytere @deepak1556
REVIEW: Address node api changes
8975bc0 
- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
deepak1556 pushed a commit to electron/electron that referenced this pull request Jan 6, 2018
@codebytere @deepak1556
REVIEW: Address node api changes
d375b50 
- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
codebytere added a commit to electron/electron that referenced this pull request Jan 6, 2018
@codebytere
Upgrade to node v9.3.0 (#11507)
bf5ecaf 
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
zcbenz pushed a commit to electron/electron that referenced this pull request Jan 31, 2018
@codebytere @zcbenz
Upgrade to node v9.3.0 (#11507)
77ec7e3 
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
alexeykuzmin pushed a commit to electron/electron that referenced this pull request Feb 21, 2018
@codebytere @alexeykuzmin
Upgrade to node v9.3.0 (#11507)
577b71a 
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
alexeykuzmin pushed a commit to electron/electron that referenced this pull request Feb 22, 2018
@codebytere @alexeykuzmin
Upgrade to node v9.3.0 (#11507)
5624070 
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
zcbenz pushed a commit to electron/electron that referenced this pull request Feb 23, 2018
@codebytere @zcbenz
Upgrade to node v9.3.0 (#11507)
6c2f7e5 
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
zcbenz pushed a commit to electron/electron that referenced this pull request Feb 23, 2018
@codebytere @zcbenz
Upgrade to node v9.3.0 (#11507)
0e5b6f9 
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
sethlu pushed a commit to sethlu/electron that referenced this pull request May 3, 2018
@codebytere @sethlu
Upgrade to node v9.3.0 (electron#11507)
c9a057c 
* update submodule refs for node v9.3.0

* Define "llvm_version" for Node.js build

* NODE_MODULE_CONTEXT_AWARE_BUILTIN -> NODE_BUILTIN_MODULE_CONTEXT_AWARE

* update NodePlatform to MultiIsolatePlatform

* fix linting error

* update node ref

* REVIEW: Explicitly register builtin modules

nodejs/node#16565

* update libcc ref

* switch libcc to c62

* REVIEW: Address node api changes

- Always start the inspector agent for nodejs/node#17085
- Set the tracing controller for node nodejs/node#15538
- Isolate data creation now requires plaform nodejs/node#16700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

@addaleax addaleax Awaiting requested review from addaleax

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

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 lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@eugeneo @apapirovski @gibfahn @jasnell @TimothyGu @mscdex @nodejs-github-bot