(vee-eight-4.9) contextify: update deprecated SetWeak usage by ofrobots · Pull Request #5392 · nodejs/node

ofrobots · 2016-02-23T17:59:50Z

Description of change

Follow on from #5204 (which is pending landing once the CI resumes).

This PR cleans up the complexity with how weakness was being used in node_contextify and updates to the new style Phantom weakness API.

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done afterwards /
while the PR is open.

Affected core subsystem(s)

contextify

R=@bnoordhuis, @indutny
/cc @nodejs/v8

bnoordhuis · 2016-02-24T12:05:51Z

src/node_contextify.cc

You can drop the explicit keyword and put everything on one line.

Cleanup how node_contextify keeps weak references in order to prepare for new style phantom weakness API. We didn't need to keep a weak reference to the context's global proxy, as the context holds it.

Simplify how node_contextify was keeping a weak reference to the sandbox object in order to prepare for new style phantom weakness V8 API. It is simpler (and more robust) for the context to hold a reference to the sandbox in an embedder data field. Doing otherwise meant that the sandbox could become weak while the context was still alive. This wasn't a problem because we would make the reference strong at that point. Since the sandbox must live at least as long as the context, it would be better for the context to hold onto the sandbox.

ofrobots · 2016-02-25T21:11:30Z

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

This is ready for review.

bnoordhuis · 2016-02-26T12:34:45Z

LGTM

Cleanup how node_contextify keeps weak references in order to prepare for new style phantom weakness API. We didn't need to keep a weak reference to the context's global proxy, as the context holds it. PR-URL: #5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Simplify how node_contextify was keeping a weak reference to the sandbox object in order to prepare for new style phantom weakness V8 API. It is simpler (and more robust) for the context to hold a reference to the sandbox in an embedder data field. Doing otherwise meant that the sandbox could become weak while the context was still alive. This wasn't a problem because we would make the reference strong at that point. Since the sandbox must live at least as long as the context, it would be better for the context to hold onto the sandbox. PR-URL: #5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: #5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

ofrobots · 2016-02-26T22:17:45Z

Thanks. Landed on vee-eight-4.9 as 7a863af...3921770.

Cleanup how node_contextify keeps weak references in order to prepare for new style phantom weakness API. We didn't need to keep a weak reference to the context's global proxy, as the context holds it. PR-URL: nodejs#5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

Simplify how node_contextify was keeping a weak reference to the sandbox object in order to prepare for new style phantom weakness V8 API. It is simpler (and more robust) for the context to hold a reference to the sandbox in an embedder data field. Doing otherwise meant that the sandbox could become weak while the context was still alive. This wasn't a problem because we would make the reference strong at that point. Since the sandbox must live at least as long as the context, it would be better for the context to hold onto the sandbox. PR-URL: nodejs#5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs#5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2016-03-21T19:19:51Z

A fix for the regression in v5.9.0 has just landed and will be included in v5.9.1

Let's wait some more to see if this ends up working as expected

ofrobots · 2016-03-21T19:47:45Z

I would also be good to wait for more feedback on #3113 before backporting to LTS.

MylesBorins · 2016-05-17T22:40:35Z

@ofrobots would you be able to prepare a backport? it looks like this solved some pretty nasty bugs 😄

edit: including --> #5800 of course 😄

ofrobots · 2016-05-17T22:49:14Z

Sure thing. I will put together a PR for 4.x, sometime later this week.

Cleanup how node_contextify keeps weak references in order to prepare for new style phantom weakness API. We didn't need to keep a weak reference to the context's global proxy, as the context holds it. PR-URL: nodejs#5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Simplify how node_contextify was keeping a weak reference to the sandbox object in order to prepare for new style phantom weakness V8 API. It is simpler (and more robust) for the context to hold a reference to the sandbox in an embedder data field. Doing otherwise meant that the sandbox could become weak while the context was still alive. This wasn't a problem because we would make the reference strong at that point. Since the sandbox must live at least as long as the context, it would be better for the context to hold onto the sandbox. PR-URL: nodejs#5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: nodejs#5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Cleanup how node_contextify keeps weak references in order to prepare for new style phantom weakness API. We didn't need to keep a weak reference to the context's global proxy, as the context holds it. PR-URL: #5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Simplify how node_contextify was keeping a weak reference to the sandbox object in order to prepare for new style phantom weakness V8 API. It is simpler (and more robust) for the context to hold a reference to the sandbox in an embedder data field. Doing otherwise meant that the sandbox could become weak while the context was still alive. This wasn't a problem because we would make the reference strong at that point. Since the sandbox must live at least as long as the context, it would be better for the context to hold onto the sandbox. PR-URL: #5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: #5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

ofrobots mentioned this pull request Feb 23, 2016

(vee-eight-4.9) Replace more deprecated V8 API usage #5204

Closed

3 tasks

ofrobots added the v8 engine Issues and PRs related to the V8 dependency. label Feb 23, 2016

bnoordhuis reviewed Feb 24, 2016
View reviewed changes

ofrobots force-pushed the setweak-contextify branch from 1d60f7a to 7d7aed8 Compare February 24, 2016 17:54

ofrobots added dont-land-on-v5.x labels Feb 25, 2016

ofrobots added 4 commits February 25, 2016 09:07

contextify: cleanup weak ref for global proxy

820a1aa

Cleanup how node_contextify keeps weak references in order to prepare for new style phantom weakness API. We didn't need to keep a weak reference to the context's global proxy, as the context holds it.

contextify: replace deprecated SetWeak usage

9f2d84a

contextify: cache sandbox and context in locals

fd21188

ofrobots force-pushed the setweak-contextify branch from 7d7aed8 to fd21188 Compare February 25, 2016 17:07

ofrobots added a commit that referenced this pull request Feb 26, 2016

contextify: replace deprecated SetWeak usage

87a0ac7

PR-URL: #5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

ofrobots added a commit that referenced this pull request Feb 26, 2016

contextify: cache sandbox and context in locals

3921770

PR-URL: #5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

ofrobots closed this Feb 26, 2016

ofrobots deleted the setweak-contextify branch February 26, 2016 22:18

ofrobots mentioned this pull request Feb 26, 2016

(vee-eight-4.9) src: update uses of deprecated NewExternal #5462

Closed

4 tasks

ofrobots added a commit to ofrobots/node that referenced this pull request Mar 1, 2016

contextify: replace deprecated SetWeak usage

14b322b

PR-URL: nodejs#5392 Reviewed-By: Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins mentioned this pull request Mar 21, 2016

Backport 5786 to v5.x #5800

Closed

3 tasks

rvagg pushed a commit to rvagg/io.js that referenced this pull request May 20, 2016

contextify: replace deprecated SetWeak usage

ab00cf5

PR-URL: nodejs#5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

rvagg pushed a commit to rvagg/io.js that referenced this pull request May 20, 2016

contextify: cache sandbox and context in locals

4d48400

PR-URL: nodejs#5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request May 20, 2016

contextify: replace deprecated SetWeak usage

8ebdcd6

PR-URL: #5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request May 20, 2016

contextify: cache sandbox and context in locals

9ddb44b

PR-URL: #5392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins mentioned this pull request May 20, 2016

v4.4.5 proposal #6824

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jun 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

(vee-eight-4.9) contextify: update deprecated SetWeak usage#5392

(vee-eight-4.9) contextify: update deprecated SetWeak usage#5392
ofrobots wants to merge 4 commits intonodejs:vee-eight-4.9from
ofrobots:setweak-contextify

ofrobots commented Feb 23, 2016

Uh oh!

bnoordhuis Feb 24, 2016

Uh oh!

ofrobots Feb 25, 2016

Uh oh!

ofrobots commented Feb 25, 2016

Uh oh!

bnoordhuis commented Feb 26, 2016

Uh oh!

ofrobots commented Feb 26, 2016

Uh oh!

MylesBorins commented Mar 21, 2016

Uh oh!

ofrobots commented Mar 21, 2016

Uh oh!

MylesBorins commented May 17, 2016 •

edited

Loading

Uh oh!

ofrobots commented May 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ofrobots commented Feb 23, 2016

Description of change

Pull Request check-list

Affected core subsystem(s)

Uh oh!

bnoordhuis Feb 24, 2016

Choose a reason for hiding this comment

Uh oh!

ofrobots Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

ofrobots commented Feb 25, 2016

Uh oh!

bnoordhuis commented Feb 26, 2016

Uh oh!

ofrobots commented Feb 26, 2016

Uh oh!

MylesBorins commented Mar 21, 2016

Uh oh!

ofrobots commented Mar 21, 2016

Uh oh!

MylesBorins commented May 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofrobots commented May 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MylesBorins commented May 17, 2016 •

edited

Loading