Skip to content

src: free up memory before re-setting URLHost value#18357

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
Closed
prog1dev wants to merge 3 commits into from
Closed

src: free up memory before re-setting URLHost value #18357

prog1dev wants to merge 3 commits into from

Conversation

@prog1dev
Copy link
Contributor

@prog1devprog1dev commented Jan 24, 2018

Resolves#18302

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • Version: master
  • Platform: all
  • Subsystem: url, src

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 24, 2018
@prog1dev
Copy link
ContributorAuthor

prog1dev commented Jan 24, 2018

@TimothyGu@apapirovski ping

TimothyGu
TimothyGu approved these changes Jan 24, 2018
View reviewed changes
Copy link
Member

@TimothyGuTimothyGu left a comment

Choose a reason for hiding this comment

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

Looks good! One tiny comment.

src/node_url.cc
case HostType::H_DOMAIN: value_.domain.~string(); break;
case HostType::H_OPAQUE: value_.opaque.~string(); break;
default: break;
}
Copy link
Member

@TimothyGuTimothyGuJan 24, 2018

Choose a reason for hiding this comment

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

After resettig the value_, can you reset the type_ as well by setting it to HostType::H_FAILED?

Copy link
ContributorAuthor

@prog1devprog1devJan 24, 2018•
edited
Loading

Choose a reason for hiding this comment

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

done a7c310f

apapirovski
apapirovski approved these changes Jan 24, 2018
View reviewed changes
Copy link
Contributor

@apapirovskiapapirovski left a comment

Choose a reason for hiding this comment

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

LGTM with @TimothyGu's nit.

@apapirovski
Copy link
Contributor

apapirovski commented Jan 24, 2018

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

@apapirovski
Copy link
Contributor

apapirovski commented Jan 25, 2018

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

(the infrastructure should be less flaky now)

@prog1devprog1dev changed the title Fix 18302src: free up memory before re-setting URLHost valueJan 25, 2018
@TimothyGu
Copy link
Member

TimothyGu commented Jan 26, 2018

The Windows issues look unrelated, as the Windows CI jobs have been unwell for a while now: https://ci.nodejs.org/job/node-test-commit-windows-fanned/

Unpinning dont-land-on-v6.x, as the URL parser will go into v6.x soon.

@TimothyGuTimothyGu added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

Landed in 36fd25f

@BridgeARBridgeAR closed this Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
@prog1dev@BridgeAR
src: free memory before re-setting URLHost value
36fd25f
Fixes: nodejs#18302 PR-URL: nodejs#18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@prog1dev
Copy link
ContributorAuthor

prog1dev commented Feb 1, 2018

yay! \o/

@prog1devprog1dev deleted the fix_18302 branch February 1, 2018 10:33
@addaleaxaddaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
@prog1dev@MylesBorins
src: free memory before re-setting URLHost value
6f2c51c
Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@prog1dev@MylesBorins
src: free memory before re-setting URLHost value
874361d
Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@prog1dev@MylesBorins
src: free memory before re-setting URLHost value
43956e9
Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@prog1dev@MylesBorins
src: free memory before re-setting URLHost value
265faf8
Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 20, 2018

This landed cleanly on v8.x

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

@prog1dev
Copy link
ContributorAuthor

prog1dev commented Mar 20, 2018•
edited
Loading

@MylesBorins How to know if this should be backported or not? Just check the code?

@tniessen
Copy link
Member

tniessen commented Mar 20, 2018•
edited
Loading

@prog1dev

  1. Will it cause problems on that branch or incompatibilities with our or third-party code, including JS modules, or is it marked as semver-major? → Don't backport.
  2. Will people using v6.x benefit from this change? → Backport.
  3. Will it make backporting other changes easier? → Backport.
  4. None of the above? → Don't backport.

@prog1dev
Copy link
ContributorAuthor

prog1dev commented Mar 20, 2018

@tniessen Shouldnt this part be in backporting-to-release-lines.md?

@prog1devprog1dev mentioned this pull request Mar 21, 2018
Closed
@TimothyGu
Copy link
Member

TimothyGu commented Mar 23, 2018

Yes, this should indeed be backported to v6.x. It doesn't fix any known bugs, but increases the robustness of the code in question.

@prog1devprog1dev mentioned this pull request Mar 27, 2018
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@prog1dev@MylesBorins
src: free memory before re-setting URLHost value
06a28aa
Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@prog1dev@MylesBorins
src: free memory before re-setting URLHost value
c0f40be
Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
prog1dev added a commit to prog1dev/node that referenced this pull request Apr 1, 2018
@prog1dev
src: free memory before re-setting URLHost value
af2c41e
Fixes: nodejs#18302 PR-URL: nodejs#18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
@prog1dev@MylesBorins
src: free memory before re-setting URLHost value
0ca2dad
Fixes: #18302 Backport-PR-URL: #19639 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 13, 2018
Merged
@MylesBorinsMylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@prog1dev@MayaLekova
src: free memory before re-setting URLHost value
23e845c
Fixes: nodejs#18302 PR-URL: nodejs#18357 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@tniessentniessentniessen approved these changes

+2 more reviewers

@apapirovskiapapirovskiapapirovski approved these changes

@TimothyGuTimothyGuTimothyGu 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++.whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

URLHost not robust when re-setting its value

9 participants

@prog1dev@apapirovski@TimothyGu@BridgeAR@MylesBorins@tniessen@jasnell@addaleax@nodejs-github-bot