src: free up memory before re-setting URLHost value by prog1dev · Pull Request #18357 · nodejs/node

prog1dev · 2018-01-24T20:04:36Z

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

Fixes: nodejs#18302

prog1dev · 2018-01-24T20:07:16Z

@TimothyGu @apapirovski ping

TimothyGu

Looks good! One tiny comment.

TimothyGu · 2018-01-24T20:09:42Z

+ case HostType::H_DOMAIN: value_.domain.~string(); break;
+ case HostType::H_OPAQUE: value_.opaque.~string(); break;
+ default: break;
+ }


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

done a7c310f

apapirovski

LGTM with @TimothyGu's nit.

apapirovski · 2018-01-24T22:25:05Z

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

apapirovski · 2018-01-25T15:57:48Z

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

(the infrastructure should be less flaky now)

TimothyGu · 2018-01-26T03:49:07Z

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.

BridgeAR · 2018-02-01T10:06:03Z

Landed in 36fd25f

Fixes: nodejs#18302 PR-URL: nodejs#18357 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

prog1dev · 2018-02-01T10:33:27Z

yay! \o/

Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

MylesBorins · 2018-03-20T15:55:44Z

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 · 2018-03-20T17:14:09Z

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

tniessen · 2018-03-20T17:28:02Z

@prog1dev

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.
Will people using v6.x benefit from this change? → Backport.
Will it make backporting other changes easier? → Backport.
None of the above? → Don't backport.

prog1dev · 2018-03-20T18:00:10Z

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

TimothyGu · 2018-03-23T00:19:33Z

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.

Fixes: #18302 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Fixes: nodejs#18302 PR-URL: nodejs#18357 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Fixes: #18302 Backport-PR-URL: #19639 PR-URL: #18357 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Fixes: nodejs#18302 PR-URL: nodejs#18357 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

prog1dev added 2 commits January 24, 2018 01:49

src: define URLHost::Reset() function
bd831b4
Fixes: nodejs#18302

src: free memory first on URLHost::SetOpaque & URLHost::SetDomain call
4a2adb0
Fixes: nodejs#18302

nodejs-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

TimothyGu approved these changes Jan 24, 2018
View reviewed changes

apapirovski approved these changes Jan 24, 2018
View reviewed changes

src: reset _type on URLHost::Reset() call
a7c310f

TimothyGu approved these changes Jan 24, 2018
View reviewed changes

prog1dev changed the title ~~Fix 18302~~src: free up memory before re-setting URLHost valueJan 25, 2018

jasnell approved these changes Jan 26, 2018
View reviewed changes

TimothyGu requested a review from addaleax January 26, 2018 03:47

TimothyGu removed the dont-land-on-v6.x label Jan 26, 2018

TimothyGu added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2018

addaleax approved these changes Jan 26, 2018
View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716Compare January 26, 2018 22:03

cjihrig force-pushed the master branch from 993b716 to 082f952Compare January 26, 2018 22:36

tniessen approved these changes Jan 29, 2018
View reviewed changes

BridgeAR closed this Feb 1, 2018

prog1dev deleted the fix_18302 branch February 1, 2018 10:33

addaleax 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 mentioned this pull request Feb 21, 2018
v9.6.0 proposal #18902
Merged

MylesBorins added backport-requested-v6.x labels Mar 20, 2018

prog1dev mentioned this pull request Mar 21, 2018
Update backporting-to-release-lines.md? #19491
Closed

prog1dev mentioned this pull request Mar 27, 2018
[v6.x backport] src: free memory before re-setting URLHost value #19639
Closed
2 tasks

MylesBorins mentioned this pull request Apr 13, 2018
v6.14.2 proposal #19996
Merged

MylesBorins mentioned this pull request May 2, 2018
v8.11.2 proposal #20478
Merged

Uh oh!

Uh oh!

Conversation

prog1dev commented Jan 24, 2018

Checklist

Affected core subsystem(s)

Uh oh!

prog1dev commented Jan 24, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGuJan 24, 2018

Choose a reason for hiding this comment

Uh oh!

prog1devJan 24, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski commented Jan 24, 2018

Uh oh!

apapirovski commented Jan 25, 2018

Uh oh!

TimothyGu commented Jan 26, 2018

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

prog1dev commented Feb 1, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

prog1dev commented Mar 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Mar 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prog1dev commented Mar 20, 2018

Uh oh!

TimothyGu commented Mar 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

prog1devJan 24, 2018•
edited
Loading

prog1dev commented Mar 20, 2018•
edited
Loading

tniessen commented Mar 20, 2018•
edited
Loading