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

deps: cherry-pick 1f53e42 from v8 upstream (v6.x) #7789

Closed
wants to merge 34 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 19, 2016

mcollina and others added 30 commits July 15, 2016 08:18
As titled. Tested by @piccoloaiutante.

PR-URL: nodejs#4647
Refs: wixtoolset/wix3#366
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Frederic Hemberger <[email protected]>
Extend linting to tools/license2rtf.js and any other JS that gets added
to the `tools` directory by default.

This incidentally simplifies lint invocation.

PR-URL: nodejs#7647
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Hopefully clarify the behaviour of `buffer.indexOf()` and
`buffer.includes()` for numbers in that they will be
truncated to uint8s.

Add tests for that behaviour.

Fixes: nodejs#7591
PR-URL: nodejs#7611
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
compliment -> complement

PR-URL: nodejs#7568
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
All quotes in .eslintrc were unnecessary and inconsistently placed
across the file. Additionally, format the globals to be consistent
with the style of whitespace and sorted them alphabetically.

PR-URL: nodejs#7691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#2569
Reviewed-By: Jackson Tian <[email protected]>
Reviewed-By: Wexpo Lyu <[email protected]>
Reviewed-By: Yiyu He <[email protected]>
Reviewed-By: Yorkie Liu <[email protected]>
Current implementation tracks connected/disconnected status separately
which potentially introduces race condition.
This change introduces notion of session IDs and also posts
connect/disconnect events into the same queue as the messages. This way
Node knows what session given response belongs to and can discard
messages if the frontend for that session had disconnected.

This also fixes an issue when frontend was unable to attach to V8
instance that was running infinite loop.

PR-URL: nodejs#7271
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
A few of the child process tests can be simplified by computing
the OS specific root directory in common and then accessing that
value.

PR-URL: nodejs#7685
Reviewed-By: Roman Reiss <[email protected]>
The name 'event' for the argument of the listener in
fs.watch was confusing considering FSWatcher also had
events. This changes the name of the argument to
eventType.

Fixes: nodejs#7504
PR-URL: nodejs#7506
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
A number of test files use IIFEs to separate distinct tests from
each other in the same file. The project has been moving toward
using block scopes and let/const in favor of IIFEs. This commit
moves IIFE tests to block scopes. Some additional cleanup such
as use of strictEqual() and common.mustCall() is also included.

PR-URL: nodejs#7694
Reviewed-By: Santiago Gimeno <[email protected]>
Provide additional information about values that indicate test failed.

PR-URL: nodejs#7693
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Brian White <[email protected]>
"doc/api/fs.md" file had some conflict markers like "<<<<<<< HEAD"
that are visible at the bottom of https://nodejs.org/api/fs.html

PR-URL: nodejs#7590
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Fixes regression where creating a new Buffer from an
empty ArrayBuffer would fail.

Ref: nodejs@85ab4a5
PR-URL: nodejs#7176
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Original commit message:

    Quit creating array literal boilerplates from Crankshaft.

    It's such a corner case.

    BUG=

    Review URL: https://codereview.chromium.org/1865013002

    Cr-Commit-Position: refs/heads/master@{nodejs#35346}

Fixes: nodejs#7454
PR-URL: nodejs#7633
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
This commit removes the use of self and bind() from the cluster
module in favor of arrow functions.

PR-URL: nodejs#7710
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Fix small typo in Buffering section of stream doc.
PR-URL: nodejs#7738
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
State in the documentation that `uncaughtException` is not a reliable
way to restart a crashed application, and clarify that an application
may crash in ways that do not trigger this event.

Use a documented synchronous function in example code.

Fixes: nodejs#6223

Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#6378
Whenever a timer is scheduled within another timer, there are a few
known issues that we are fixing:

* Whenever the timer being scheduled has the same timeout value as the
outer timer, the newly created timer can fire on the same tick of the
event loop instead of during the next tick of the event loop
* Whenever a timer is added in another timer's callback, its underlying
timer handle will be started with a timeout that is actually incorrect

This commit consists of
nodejs/node-v0.x-archive#17203 and
nodejs/node-v0.x-archive#25763.

Fixes: nodejs/node-v0.x-archive#9333
Fixes: nodejs/node-v0.x-archive#15447
Fixes: nodejs/node-v0.x-archive#25607
Fixes: nodejs#5426
PR-URL: nodejs#3063
Adds missing semicolons, removes extra white space, and properly indents
various code snippets in the documentation.

Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
PR-URL: nodejs#7745
Many tests use assert.fail(null, null, msg) where it would be
simpler to use common.fail(msg). This is largely because
common.fail() is fairly new. This commit makes the replacement
when applicable.

PR-URL: nodejs#7735
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
`ml64.exe` doesn't support `/safeseh` option. Do not attempt to use it
if `target_arch=="x64"`.

See: https://msdn.microsoft.com/en-us/library/s0ksfwcf.aspx
PR-URL: nodejs#7759
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: nodejs#7639
PR-URL: nodejs#7641
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: nodejs#7602
Reviewed-By: Anna Henningsen <[email protected]>
Using identical timeout values appears to have eliminated the flakiness
in the test.

Fixes: nodejs#7643
PR-URL: nodejs#7717
Reviewed-By: Fedor Indutny <[email protected]>
Ref: nodejs#6578
PR-URL: nodejs#7287
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Claudio Rodriguez <[email protected]>
Fix handle leaks in Buffer::New() and Buffer::Copy() by creating the
handle scope before looking up the env with Environment::GetCurrent().

Environment::GetCurrent() calls v8::Isolate::GetCurrentContext(), which
creates a handle in the current scope, i.e., the scope created by the
caller of Buffer::New() or Buffer::Copy().

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Create a handle scope before performing a check that creates a handle,
otherwise the handle is leaked into the handle scope of the caller.

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Create a handle scope before performing a check that creates a handle,
otherwise the handle is leaked into the handle scope of the caller.

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
API function callbacks run inside an implicit HandleScope.  We don't
need to explicitly create one and in fact introduce some unnecessary
overhead when we do.

PR-URL: nodejs#7711
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Comparing the buffers `ABC` and `ABCD` returns `-1` not `1`.

PR-URL: nodejs#7777
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Trott and others added 3 commits July 19, 2016 05:45
Update some outdated material. Provide some minor fixes. Wrap to 80
characters.

PR-URL: nodejs#7719
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Julien Gilli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Remove wtf files as v8_inspector no longer needs them.

Ref: nodejs#7123

PR-URL: nodejs#7751
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Original commit message:

    Handle symbols in FrameMirror#invocationText().

    Fix a TypeError when putting together the invocationText for a
    symbol method's stack frame.

    See nodejs#7536.

    Review-Url: https://codereview.chromium.org/2122793003
    Cr-Commit-Position: refs/heads/master@{nodejs#37597}

Fixes: nodejs#7536
PR-URL: nodejs#7612
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@bnoordhuis bnoordhuis added v8 engine Issues and PRs related to the V8 dependency. v6.x labels Jul 19, 2016
@evanlucas
Copy link
Contributor

Trying CI one more time, plinux build failed

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

@evanlucas
Copy link
Contributor

evanlucas commented Jul 19, 2016

Rubberstamp LGTM though. Thanks for backporting!

@bnoordhuis
Copy link
Member Author

Another build error on https://ci.nodejs.org/job/node-test-commit-plinux/3308/nodes=ppcbe-ubuntu1404/console:

/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcbe-ubuntu1404/out/Release/obj.target/node/src/node_main.o: file not recognized: File truncated

@mhdawson Can you comment? Disk full, perhaps?

@bnoordhuis
Copy link
Member Author

The ARM failures are parallel/test-fs-readfile-tostring-fail and parallel/test-fs-read-buffer-tostring-fail which should have been fixed by #7042, IIRC.

@Trott Can you comment? This PR is on top of commit 8e50413.

@Trott
Copy link
Member

Trott commented Jul 20, 2016

@bnoordhuis That needs #7772 (to increase common.enoughTestMem to require 1 Gb instead of 512 Mb) which hasn't landed yet but, uh, I'll go land that one right now...

EDIT: ...and now it's landed in 135a863.

@mhdawson
Copy link
Member

I'm not sure what's up with the PPC failure, I've run a few jobs through without any problems. If it is consistently occurring with just this PR then I'd be concerned.

@mhdawson
Copy link
Member

I'm ( wondering if some other commit may have broken 6.x for PPC. CI run against 6.x branch also failed (https://ci.nodejs.org/job/node-test-commit-plinux/nodes=ppcbe-ubuntu1404/3326/) Now running one against head (https://ci.nodejs.org/job/node-test-commit-plinux/3327/nodes=ppcbe-ubuntu1404/) to see if that passes or not. If it passes then I'm thinking we might have a real issues versus a machine issue. Run against head seems ok.

Launching one against v6.3 to confirm that its something between v6.3 and what is currently in v6.x - https://ci.nodejs.org/job/node-test-commit-plinux/3331/nodes=ppcbe-ubuntu1404/console

@mhdawson
Copy link
Member

v6.3.0 seemed to be fine on same machine were we've seen the failures so must be some commit between v6.3.0 and the current v6.x contents.

@evanlucas
Copy link
Contributor

I'll be digging into this tonight to see what landed on v6.x that is breaking it

@mhdawson
Copy link
Member

mhdawson commented Jul 20, 2016

Well last run on v6.x seems to be ok, https://ci.nodejs.org/job/node-test-commit-plinux/nodes=ppcbe-ubuntu1404/3334/console, a bit frustrating.

@mhdawson
Copy link
Member

I don't think there are any disk space issues on either machine, but I'm going to try flushing the workspace and running against the PR again to see if that makes any difference

@mhdawson
Copy link
Member

Still failed against this PR. Disabled be-1 to force onto be-2 to see what happens when it runs there. Still don't understand why we seem to pass on the same machine for the 6.x and 6.3.0 builds but lets see how running this PR on be-2 goes. The only other difference I can think of is that for this PR I'm launching through the test-pull-request job versus the node-test-commit-plinux.

@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

So run on be-2 was ok. Going to reboot be-1 and see if that makes any difference.

@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

still failed after reboot

@mhdawson
Copy link
Member

Going to re-run ansible to update to see if that helps.

@mhdawson
Copy link
Member

In any case looking more like something on the machine since passed on be-2 so sorry for any delay its caused on this PR.

@mhdawson
Copy link
Member

Still failed after re-running ansible to update on be-1. File it is complaining about is size 0 after the failure but when I run the g++ command manually it seems to be ok. I can't quite explain why it only occurs on this PR consistently, is fine on master, and is fine on be-2. If I locally run make after the failure on be-1 it builds ok and then the tests run as well.

At this point I'm still wondering what else to do, the only thing left would be to destroy/re-image the machine. I'd say land the PR (assuming there are not failures on other platforms) and I'll see if we see this re-occur on other regular runs and if so kill/recreate the machine.

@evanlucas
Copy link
Contributor

Landed in v6.x in d3f0a6a. Thanks!

@evanlucas evanlucas closed this Jul 21, 2016
@fhinkel
Copy link
Member

fhinkel commented Jul 21, 2016

CI had the same problem for #7638.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.