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.

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

src: fix v8 local handling in node_url.cc #11064

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jan 29, 2017

Calling .As<Object>() on a non-object aborts in debug mode, but node_url.cc relied on it. Address that by using Local<Value> until it has been confirmed that the handle actually is an object.

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

src / whatwg-url

CI: https://ci.nodejs.org/job/node-test-commit/7535/

Calling `.As<Object>()` on a non-object aborts in debug mode,
but `node_url.cc` relied on it. Address that by using `Local<Value>`
until it has been confirmed that the handle actually is an object.
@nodejs-github-bot 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 29, 2017
@targos
Copy link
Member

targos commented Jan 29, 2017

It's perhaps a good idea to have one CI host in debug mode.

@addaleax
Copy link
Member Author

@targos The tests fail in debug mode anyway, due to #7144 and c2c6ae5#commitcomment-20653421 :/

This one was just easy to fix for me.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

jasnell pushed a commit that referenced this pull request Jan 30, 2017
Calling `.As<Object>()` on a non-object aborts in debug mode,
but `node_url.cc` relied on it. Address that by using `Local<Value>`
until it has been confirmed that the handle actually is an object.

PR-URL: #11064
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 30, 2017

Landed in 0d290a2

@jasnell jasnell closed this Jan 30, 2017
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
Calling `.As<Object>()` on a non-object aborts in debug mode,
but `node_url.cc` relied on it. Address that by using `Local<Value>`
until it has been confirmed that the handle actually is an object.

PR-URL: #11064
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants