Annotate the benign race in ScriptForbiddenScope::isScriptForbidden().

Annotate the benign race in ScriptForbiddenScope::isScriptForbidden().

BUG=700713

[esprehn, 2017-03-17 02:19:49]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 02:20:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2017-03-17 03:51:08]

esprehn@chromium.org changed reviewers:
+ inferno@chromium.org

[inferno, 2017-03-17 04:05:16]

inferno@chromium.org changed reviewers:
+ glider@chromium.org

[inferno, 2017-03-17 04:05:17]

+cc Alex, our TSAN expert who can review this.

[commit-bot: I haz the power, 2017-03-17 04:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 04:42:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Potapenko, 2017-03-17 10:14:50]

Not lgtm

https://codereview.chromium.org/2753203002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/ScriptForbiddenScope.h (right):

https://codereview.chromium.org/2753203002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/ScriptForbiddenScope.h:56:
WTF_ANNOTATE_BENIGN_RACE(&s_scriptForbiddenCount,
Please no.
WTF_ANNOTATE_BENIGN_RACE is a beast we need to get rid of, not let it grow.
If you're reading something from several threads, these reads must be
synchronized.

https://codereview.chromium.org/2753203002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/ScriptForbiddenScope.h:59: return
s_scriptForbiddenCount && isMainThread();
If the value only matters on the main thread, how about we read it on the main
thread only?
Putting isMainThread() on the first place should solve the problem:

  return isMainThread() && s_scriptForbiddenCount;

If it does not, someone might be modifying s_scriptForbiddenCount from another
thread.

[Alexander Potapenko, 2017-03-17 10:41:26]

For the record, I've filed https://crbug.com/702558 to remove all benign race
annotations from the codebase.

[esprehn, 2017-03-17 12:48:34]

Explain why this is not safe? In all the common cases this value is zero. If
it's not zero we only care about it on the main thread. Reading a value has no
side effects on the underlying memory so I'm not sure why this wouldn't be safe.

I swapped these on purpose to avoid the TLS lookup on main which is important.
There's no way to only read it on main, the script execution code is the same on
all threads.

[Alexander Potapenko, 2017-03-17 13:05:57]

On 2017/03/17 12:48:34, esprehn wrote:
> Explain why this is not safe?
Because this is undefined behavior from the C++ Standard point of view.
The compiler is free to assume your program does not have races, and break it
otherwise.
And compilers are only getting smarter these days.

> In all the common cases this value is zero. If
> it's not zero we only care about it on the main thread. Reading a value has no
> side effects on the underlying memory so I'm not sure why this wouldn't be
safe.
This would've been true if you were writing in assembly, where you control every
memory access and their ordering, but not in C++.
Some of the consequences are described in
http://static.usenix.org/event/hotpar11/tech/final_files/Boehm.pdf and
https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-coul...

> I swapped these on purpose to avoid the TLS lookup on main which is important.
> There's no way to only read it on main, the script execution code is the same
on
> all threads.
If you really want to read this variable every time, you can use relaxed atomics
(they are called nobarrier in base::subtle).
If s_scriptForbiddenCount has state associated with it (probably it hasn't), you
need acquire/release atomics instead.