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

Issue 1258303004: Avoid data race when writing Shell::options.script_executed. (Closed)

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

Description

Avoid data race when writing Shell::options.script_executed. The race occurred when Workers were used. Since Workers call Shell::ExecuteString from a different thread, TSAN (correctly) flags this as a racy write. Solution would be to either synchronize the writes, or to 'lift' the write higher up in the call stack and only write the flag from the main thread. This implements this latter solution. These methods call Shell::ExecuteString, but do *not* set script_executed: - ExecuteInThread: Can only occur is JS has already been executed. - Shell::Load: Callback for JS; so JS has already been executed when we get there. - Shell::RunShell: Interactive shell. We no longer need script_executed once we're here. BUG=v8:4330 LOG=N Committed: https://crrev.com/e045b78d8ed1d0450f70baa44d16eda78f680188 Cr-Commit-Position: refs/heads/master@{#30003}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -1 line) Patch
M src/d8.cc View 3 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 11 (4 generated)
vogelheim
5 years, 4 months ago (2015-08-04 14:08:49 UTC) #2
Yang
On 2015/08/04 14:08:49, vogelheim wrote: lgtm.
5 years, 4 months ago (2015-08-04 14:10:12 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258303004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258303004/1
5 years, 4 months ago (2015-08-04 14:25:16 UTC) #5
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4678)
5 years, 4 months ago (2015-08-04 14:28:07 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1258303004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1258303004/1
5 years, 4 months ago (2015-08-04 14:30:15 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 4 months ago (2015-08-04 14:31:44 UTC) #10
commit-bot: I haz the power
5 years, 4 months ago (2015-08-04 14:32:39 UTC) #11
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/e045b78d8ed1d0450f70baa44d16eda78f680188
Cr-Commit-Position: refs/heads/master@{#30003}

Powered by Google App Engine
This is Rietveld 408576698