Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(65)

Issue 1223813008: Fix bug when transferring SharedArrayBuffer to multiple Workers. (try 2) (Closed)

Created:
5 years, 5 months ago by binji
Modified:
5 years, 5 months ago
Reviewers:
Jarin
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

d8 workers: Fix transferring SharedArrayBuffer to multiple Workers. (try 2) Note: the previous try was reverted for occasional flaky tests. This continued after the revert, and should be fixed by https://codereview.chromium.org/1226143003. Previously, the serialization code would call Externalize for every transferred ArrayBuffer or SharedArrayBuffer, but that function can only be called once. If the buffer is already externalized, we should call GetContents instead. Also fix use-after-free bug when transferring ArrayBuffers. The transferred ArrayBuffer must be internalized in the new isolate, or be managed by the Shell. The current code gives it to the isolate externalized and frees it immediately afterward when the SerializationData object is destroyed. BUG=chromium:497295 R=jarin@chromium.org LOG=n Committed: https://crrev.com/5a9722b2ab107b0b8d97f925609d9d2f2939eecc Cr-Commit-Position: refs/heads/master@{#29658}

Patch Set 1 #

Patch Set 2 : Remove printf in ~Worker #

Patch Set 3 : merge tsan fixes #

Patch Set 4 : more tsan fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -68 lines) Patch
M src/d8.h View 1 2 2 chunks +3 lines, -4 lines 0 comments Download
M src/d8.cc View 1 2 3 10 chunks +44 lines, -33 lines 0 comments Download
M test/mjsunit/d8-worker-sharedarraybuffer.js View 1 chunk +69 lines, -31 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 6 (2 generated)
Jarin
lgtm
5 years, 5 months ago (2015-07-08 16:32:26 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1223813008/60001
5 years, 5 months ago (2015-07-14 19:29:38 UTC) #4
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 5 months ago (2015-07-14 19:56:52 UTC) #5
commit-bot: I haz the power
5 years, 5 months ago (2015-07-14 19:57:06 UTC) #6
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/5a9722b2ab107b0b8d97f925609d9d2f2939eecc
Cr-Commit-Position: refs/heads/master@{#29658}

Powered by Google App Engine
This is Rietveld 408576698