deps: cherry-pick b93c80a from v8 upstream

[matthewloring]

Checklist

• make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

deps

Description of change

Original commit message:

If we can't rehash the backing store for weak sets & maps, do a last
resort GC

BUG=v8:4909
R=hpayer@chromium.org

Committed: https://crrev.com/b93c80a6039c757723e70420ae73375b5d277814
Cr-Commit-Position: refs/heads/master@{#37591}

Fixes: #6180

[matthewloring]

Resolution of #6398.
\cc @nodejs/v8

[ofrobots]

/cc @nodejs/v8.

• Added lts-watch-4.x label. v4.x would also need the fixes in deps: backport e093a04, 09db540 from upstream V8 #6398, but are not needed on master.
• @matthewloring: for V8 5.2 and 5.3 can you request a merge in the upstream repository.

[MylesBorins]

@ofrobots would you be willing to put together a PR against v4.x-staging that includes all of those changes?

[ofrobots]

@thealphanerd Sure. Once this has baked for a couple of weeks on master and v6.x?

[MylesBorins]

sounds reasonable to me

[bnoordhuis]

The call to HasSufficientCapacity() with n == NumberOfElements() + 1 doesn't look obviously correct to me. Here is its implementation:

template <typename Derived, typename Shape, typename Key> bool HashTable<Derived, Shape, Key>::HasSufficientCapacity(int n) { int capacity = Capacity(); int nof = NumberOfElements() + n; int nod = NumberOfDeletedElements(); // Return true if: // 50% is still free after adding n elements and // at most 50% of the free elements are deleted elements. if (nod <= (capacity - nof) >> 1) { int needed_free = nof >> 1; if (nof + needed_free <= capacity) return true; } return false; }

capacity - nof can be < 0 if capacity < NumberOfElements() * 2 + 1 and right-shifting a negative number has implementation dependent behavior. I think compilers all implement it as an arithmetic shift but I'd say it's better to avoid it altogether.

As well, because it uses int, there is a potential for integer overflow when NumberOfElements() is large.

[jeisinger]

The maximum size of the backing store is 128MB so that this can never happen.

[bnoordhuis]

You mean integer overflow? What about the capacity < NumberOfElements() * 2 + 1 case?

[jeisinger]

JSWeakCollection uses an ObjectHashTable as backing store. ObjectHashTableShape::kEntrySize is 2, ObjectHashTable::kMaxCapacity (FixedArray::kMaxSize(128 MB * kPointerSize) - FixArray::kHeaderSize) / kPointerSize / kEntrySize which is a bit less than 64MB, and NumberOfElements() * 2 + 1 can be at most 134217729 which fits nicely into a signed 32bit integer.

[bnoordhuis]

Sorry, I understand that the integer overflow case is covered. I mean the right shift when capacity < nof.

[jeisinger]

Ah, sorry, I didn't read carefully enough what you actually wrote. https://codereview.chromium.org/2162333002 should address this.

[matthewloring]

I can bring in 2162333002 as part of this PR. Does the fix there look good to you @bnoordhuis?

[bnoordhuis]

Looks good at a quick glance.

[matthewloring]

Uploaded.

[matthewloring]

/cc @jeisinger who did the original PR.

[bnoordhuis]

LGTM

[matthewloring]

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

[bnoordhuis]

Can you also run the V8 test suite? That's the https://ci.nodejs.org/job/node-test-commit-v8-linux/ job.

[ofrobots]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/231/

[matthewloring]

@mhdawson @joransiu Any thoughts on the s390x failures?

[joransiu]

@matthewloring The s390x failures in StackAlignment in test-platform.cc (https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel72-s390x,v8test=v8test/231/console) should be fixed via #7771, which should have landed already.

[matthewloring]

Running after rebase.
CI: https://ci.nodejs.org/job/node-test-pull-request/3407/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/238/

[Trott]

Will probably want to re-run CI after #7873 lands (hopefully within the hour). That will fix the bad build on four of the Linux hosts.

[matthewloring]

Only the windows CI is failing and it seems to be failing similarly in other builds. I'll land this tomorrow if nobody objects.

[Trott]

Looks like the Windows issue has been fixed. Fingers-crossed re-run of CI: https://ci.nodejs.org/job/node-test-pull-request/3415/

[matthewloring]

Landed in a3d62bd, e22ffef.

[ofrobots]

@matthewloring do you also want to take care of this:

@thealphanerd

would you be willing to put together a PR against v4.x-staging that includes all of those changes?

[matthewloring]

@ofrobots @thealphanerd Opened here: #7883.