IndexedDB: Fixed threading bugs with use of AtomicStrings.

IndexedDB: Fixed threading bugs with use of AtomicStrings.
    
Some IndexedDB classes (IDBCursor, IDBRequest, IDBTransaction, and
IDBVersionChangeEvent) were using static local AtomicStrings on different
threads. This is safe, as long as these static strings are properly
created - which they were not. Switching to an initialization mechanism
which mirrors the Core static strings fixes this.

BUG=393728
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180205

[jsbell, 2014-07-31 22:53:39]

Wow...

(1) Is it even worth it to have members, vs. just `return "next"` where
appropriate?

(2) Does this affect other places where we have static local atomic strings,
i.e. IDBRequest, IDBTransaction, IDBVersionChangeEvent?

(3) Are there places outside IDB that are affected as well, or are we the only
API that exists in Workers that uses strings like this?

[jsbell, 2014-07-31 22:56:31]

On 2014/07/31 22:53:39, jsbell wrote:
> (3) Are there places outside IDB that are affected as well, or are we the only
> API that exists in Workers that uses strings like this?

I see some uses in core/fetch (which will be window+workers). All the others
look like they're on the main thread only.

Is there an IDB-specific use pattern that's vulnerable, or is the initialization
enough?

[jsbell, 2014-08-01 18:18:36]

Looks like DEFINE_STATIC_LOCAL is inherently threadsafe if-and-only-if the
constructor itself is threadsafe. But that's not the case for AtomicString's
constructor which mutates a shared hashtable.

Rather than members, can we just make these Strings instead of AtomicStrings?

We should also fix the other places in IDB and track down other possible
cross-thread uses of AtomicString.

[cmumford, 2014-08-04 16:34:46]

On 2014/08/01 18:18:36, jsbell (OOO through Aug 10) wrote:
> Looks like DEFINE_STATIC_LOCAL is inherently threadsafe if-and-only-if the
> constructor itself is threadsafe. But that's not the case for AtomicString's
> constructor which mutates a shared hashtable.
> 
> Rather than members, can we just make these Strings instead of AtomicStrings?
> 
> We should also fix the other places in IDB and track down other possible
> cross-thread uses of AtomicString.

Yes, I see your point. I'll give that a try and will also remove all other
static global IDB strings. I like the idea of doing that since it'll reduce
memory consumption *and* I don't believe there is any contract specifying how
long the caller of direction() can keep the reference to the returned String.

[cmumford, 2014-08-04 20:09:47]

Josh:

Here's the version with all IDB classes converted to WTF::String. Two things:

1) I'm not 100% certain this fixes the bug. Yes, AtomicString does access a
table, but I believe this is a thread specific table. I'm not sure what'll
happen if an AtomicString is created on one thread and accessed on another.

2) I do believe that other parts of Blink might also suffer from this bug when
used by a worker thread. Any suggestions as to how I can identify which API's
can be used by a worker?

[cmumford, 2014-08-06 16:17:18]

On 2014/08/04 20:09:47, cmumford wrote:
> Josh:
> 
> Here's the version with all IDB classes converted to WTF::String. Two things:
> 
> 1) I'm not 100% certain this fixes the bug. Yes, AtomicString does access a
> table, but I believe this is a thread specific table. I'm not sure what'll
> happen if an AtomicString is created on one thread and accessed on another.
> 
> 2) I do believe that other parts of Blink might also suffer from this bug when
> used by a worker thread. Any suggestions as to how I can identify which API's
> can be used by a worker?

Have verified that the bug is not reproducible with patch #2 on Windows -
previously it was 100% reproducible.

[jsbell, 2014-08-11 19:18:57]

On 2014/08/06 16:17:18, cmumford wrote:
> On 2014/08/04 20:09:47, cmumford wrote:
> > Josh:
> > 
> > Here's the version with all IDB classes converted to WTF::String. Two
things:
> > 
> > 1) I'm not 100% certain this fixes the bug. Yes, AtomicString does access a
> > table, but I believe this is a thread specific table. I'm not sure what'll
> > happen if an AtomicString is created on one thread and accessed on another.

Some digging turned up this doc by abarth@:

https://docs.google.com/document/d/1kOCUlJdh2WJMJGDf-WoEQhmnjKLaOYRbiHz5TiGJl...

The AtomicStrings definitely can't be used across threads; the pointer
comparisons will be bogus since the tables are per-thread (plus racy
initialization/destruction). The String usage is also not safe across threads
(see the "Threading" section of the doc) but it probably doesn't manifest (i.e.
tests pass) because the strings are never modified.

So the current patch looks insufficient...

The doc mentions "Static Strings" which are threadsafe; can you see if you can
tell how those manifest in the codebase? Those sound like what we should be
using.

> > 
> > 2) I do believe that other parts of Blink might also suffer from this bug
when
> > used by a worker thread. Any suggestions as to how I can identify which
API's
> > can be used by a worker?

Not reliably; most of the DOM (HTML+SVG elements and such) are not exposed, but
many other (network fetches) are, and I know we've moved some HTML parsing to
threads (which is mentioned in that Static String section). Grepping over
Source/ I see 399 uses of DEFINE_STATIC_LOCAL for string types, 143 are String /
256 are AtomicString, and (a subset of those) 38/128 are const String/const
AtomicString. 

Once we have the IDB code sorted out we may want to ping abarth@ and see if the
above look correct.

> 
> Have verified that the bug is not reproducible with patch #2 on Windows -
> previously it was 100% reproducible.

As noted above, this is probably technically safe (since the static strings are
initialized in a non-racy way, and never modified) but violates general
cross-thread restrictions on Strings, so we should ensure we're doing it in an
"approved" way.

[cmumford, 2014-08-12 17:44:55]

https://codereview.chromium.org/426063010/diff/40001/Source/modules/modules_g...
File Source/modules/modules_generated.gyp (right):

https://codereview.chromium.org/426063010/diff/40001/Source/modules/modules_g...
Source/modules/modules_generated.gyp:25: '../core/css/CSSGrammar.y',
I don't believe that CSSGrammar.y is required here. It's here now because I
moved this target (unmodified) from modules.gyp, but my guess is that it was
incorrect there. Probably a cut-and-paste error when that was (probably) copied
from core_generated.gyp - which does do CSS stuff.

I have no idea is XPathGrammar.y is required.

[jsbell, 2014-08-12 18:46:13]

lgtm with a few suggestions.

abarth@ should probably review as well.

https://codereview.chromium.org/426063010/diff/40001/Source/core/Init.h
File Source/core/Init.h (right):

https://codereview.chromium.org/426063010/diff/40001/Source/core/Init.h#newco...
Source/core/Init.h:42: // FIXME: Why is this function static?
... because the initializer instance is held as a static local in
initializeWithoutV8, so there's no convenient reference to it.

I don't have a good suggestion for rewriting the FIXME other than "this is
asymmetrical and that's usually a bad code smell"

https://codereview.chromium.org/426063010/diff/40001/Source/modules/BUILD.gn
File Source/modules/BUILD.gn (right):

https://codereview.chromium.org/426063010/diff/40001/Source/modules/BUILD.gn#...
Source/modules/BUILD.gn:94: "$blink_modules_output_dir/IndexedDBNames.cpp",
Should we generate into an indexeddb/ subdir, so that include paths elsewhere
are "modules/indexeddb/IndexedDBNames.h" ?

I don't think we do that for other generated code (e.g. for events and bindings)
so probably *no*, but wanted to raise it as an option.

https://codereview.chromium.org/426063010/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IndexedDBNames.in (right):

https://codereview.chromium.org/426063010/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IndexedDBNames.in:3: done
Can we group/comment these, i.e. indicating which IDL enum they come from?

https://codereview.chromium.org/426063010/diff/40001/Source/modules/modules_g...
File Source/modules/modules_generated.gyp (right):

https://codereview.chromium.org/426063010/diff/40001/Source/modules/modules_g...
Source/modules/modules_generated.gyp:25: '../core/css/CSSGrammar.y',
Agreed, looks like the modules.gyp bison rules are just copy/paste of
core_generated.gyp and can probably be removed.

[cmumford, 2014-08-12 19:30:41]

+abarth for a review of the rest of the files. abarth: Please see patch set #3
(#ps40001) for a question I had on Source/modules/modules_generated.gyp to see
if you concur.

https://codereview.chromium.org/426063010/diff/40001/Source/core/Init.h
File Source/core/Init.h (right):

https://codereview.chromium.org/426063010/diff/40001/Source/core/Init.h#newco...
Source/core/Init.h:42: // FIXME: Why is this function static?
Fixing that was unrelated to my change so I decided to leave it alone.

On 2014/08/12 18:46:12, jsbell wrote:
> ... because the initializer instance is held as a static local in
> initializeWithoutV8, so there's no convenient reference to it.
> 
> I don't have a good suggestion for rewriting the FIXME other than "this is
> asymmetrical and that's usually a bad code smell"

https://codereview.chromium.org/426063010/diff/40001/Source/modules/BUILD.gn
File Source/modules/BUILD.gn (right):

https://codereview.chromium.org/426063010/diff/40001/Source/modules/BUILD.gn#...
Source/modules/BUILD.gn:94: "$blink_modules_output_dir/IndexedDBNames.cpp",
Good question - I had the same thought and nearly asked you the same question.
We don't do it for other auto-generated code, but it would be more symmetrical
if we did. The header includes do look a bit odd without an indexeddb subdir.

On 2014/08/12 18:46:12, jsbell wrote:
> Should we generate into an indexeddb/ subdir, so that include paths elsewhere
> are "modules/indexeddb/IndexedDBNames.h" ?
> 
> I don't think we do that for other generated code (e.g. for events and
bindings)
> so probably *no*, but wanted to raise it as an option.

https://codereview.chromium.org/426063010/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IndexedDBNames.in (right):

https://codereview.chromium.org/426063010/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IndexedDBNames.in:3: done
On 2014/08/12 18:46:12, jsbell wrote:
> Can we group/comment these, i.e. indicating which IDL enum they come from?

Done.

[abarth-chromium, 2014-08-12 19:51:13]

Can we split this CL into two pieces:

1) The first patch restructures initialization.
2) The second patch adds these generated strings for IDB.

?

https://codereview.chromium.org/426063010/diff/60001/Source/modules/InitModul...
File Source/modules/InitModules.cpp (right):

https://codereview.chromium.org/426063010/diff/60001/Source/modules/InitModul...
Source/modules/InitModules.cpp:20: if (m_isInited)
Can we ASSERT(!m_isInited) ?

https://codereview.chromium.org/426063010/diff/60001/Source/modules/InitModul...
Source/modules/InitModules.cpp:30: CoreInitializer::initEventNames();
Instead, we should make an orderly init process.  That means
ModulesInitializer::init should call void ModulesInitializer::initEventNames,
not the reverse.

[cmumford, 2014-08-12 19:52:34]

https://codereview.chromium.org/426063010/diff/60001/Source/modules/modules_g...
File Source/modules/modules_generated.gyp (right):

https://codereview.chromium.org/426063010/diff/60001/Source/modules/modules_g...
Source/modules/modules_generated.gyp:19:
'../core/core_generated.gyp:core_event_interfaces',
I just noticed core_event_interfaces - I don't believe this is necessary either.
Can anybody confirm?

[abarth-chromium, 2014-08-12 20:33:00]

https://codereview.chromium.org/426063010/diff/60001/Source/modules/modules_g...
File Source/modules/modules_generated.gyp (right):

https://codereview.chromium.org/426063010/diff/60001/Source/modules/modules_g...
Source/modules/modules_generated.gyp:19:
'../core/core_generated.gyp:core_event_interfaces',
This seems unlikely to be necessary.

[cmumford, 2014-08-13 15:59:19]

This is the first patch set after factoring out the module initialization
changes into a separate CL (r180130) - no other changes aside from the
description.

[jsbell, 2014-08-13 16:39:39]

still lgtm, although I'm not confident in my ability to spot gyp issues.

[abarth-chromium, 2014-08-13 17:59:31]

LGTM

[cmumford, 2014-08-13 18:04:18]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-08-13 18:05:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cmumford@chromium.org/426063010/100001

[commit-bot: I haz the power, 2014-08-13 20:53:25]

Committed patchset #6 (100001) as 180205