Don't set WTF::Partitions::s_initialized to true before initializing

Don't set WTF::Partitions::s_initialized to true before initializing

In theory, a simultaneous caller to Partitions::getBufferPartition()
could see the flag as being true, and return the uninitialized buffer
partition, while another thread is initializing it.

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

[Jens Widell, 2014-05-02 10:10:28]

Please take a look.

One could imagine that this could lead to start-up race crashes.  No idea if it
causes any of the problems we're seeing.

[Jens Widell, 2014-05-02 10:27:25]

On 2014/05/02 10:10:28, Jens Lindström wrote:
> Please take a look.
> 
> One could imagine that this could lead to start-up race crashes.  No idea if
it
> causes any of the problems we're seeing.

Actually, it might be even better to set the flag after the spinLockUnlock()
call, since that call acts as a memory barrier and thus ought to ensure that
no-one sees uninitialized data after seeing a truthy flag.  It doesn't really
matter if all callers of initialize() set the flag, as long as only one caller
does the actual initialization.

OTOH, I'm quite outside of my comfort zone here; I haven't dealt with threading
primitives on this level before.  :-)

[eseidel, 2014-05-02 13:47:22]

I doubt this does anything since this was already inside the lock, unless the
initialize() method itself can call back into this (which presumably would
deadlock?).

Could you instead remove the check to s_isInitialized in getBufferPartitions and
turn that into a RELEASE_ASSERT(s_initialized) (or even ASSERT we shouldn't even
need to check it).

[Jens Widell, 2014-05-02 15:02:14]

On 2014/05/02 13:47:22, eseidel wrote:
> I doubt this does anything since this was already inside the lock, unless the
> initialize() method itself can call back into this (which presumably would
> deadlock?).

The lock guards against double initialization, but not against one thread
reading the flag while another thread is doing the initialization.

> Could you instead remove the check to s_isInitialized in getBufferPartitions
and
> turn that into a RELEASE_ASSERT(s_initialized) (or even ASSERT we shouldn't
even
> need to check it).

Crashes Chrome with this stack:

#0  0x0000555557d4b2fb in WTF::Vector<v8::Extension*, 0ul,
WTF::DefaultAllocator>::reserveCapacity(unsigned long) ()
#1  0x0000555557d4b45a in void WTF::Vector<v8::Extension*, 0ul,
WTF::DefaultAllocator>::appendSlowCase<v8::Extension*>(v8::Extension* const&) ()
#2  0x0000555557d4b503 in
WebCore::ScriptController::registerExtensionIfNeeded(v8::Extension*) ()
#3  0x00005555593845fd in
extensions::Dispatcher::Dispatcher(extensions::DispatcherDelegate*) ()
#4  0x000055555649eab8 in ChromeContentRendererClient::RenderThreadStarted() ()
#5  0x00005555592b2d95 in content::RenderThreadImpl::Init() ()
#6  0x00005555565be1d0 in base::Thread::ThreadMain() ()
#7  0x00005555565bee9a in base::(anonymous namespace)::ThreadFunc(void*) ()
#8  0x00007ffff32be182 in start_thread (arg=0x7fffd3c81700) at
pthread_create.c:312
#9  0x00007ffff145d30d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111

(with --single-process, but renderers crashed without --single-process.)  This
seems an obvious violation of the

  // This function must be called exactly once from the main thread before using
anything else in WTF.

comment about initialize() in WTF.h, so is probably something that should be
looked into.

[Jens Widell, 2014-05-02 15:21:08]

On 2014/05/02 15:02:14, Jens Lindström wrote:
> On 2014/05/02 13:47:22, eseidel wrote:
> > I doubt this does anything since this was already inside the lock, unless
the
> > initialize() method itself can call back into this (which presumably would
> > deadlock?).
> 
> The lock guards against double initialization, but not against one thread
> reading the flag while another thread is doing the initialization.

For the record: I also doubt this patch fixes any real issues.

I just spotted something that looked like a thread-safety bug in code that was
apparently meant to be thread-safe, while reading said code trying to spot any
kind of bugs.

And the proposed fix is fairly simple.  :-)

[eseidel, 2014-05-29 00:32:05]

lgtm

[commit-bot: I haz the power, 2014-05-29 00:32:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/268673012/1

[commit-bot: I haz the power, 2014-05-29 01:49:12]

Change committed as 175020

[Chris Evans, 2014-05-29 04:32:30]

On 2014/05/02 10:10:28, Jens Lindström wrote:
> Please take a look.
> 
> One could imagine that this could lead to start-up race crashes.  No idea if
it
> causes any of the problems we're seeing.

[+tsepez]

I only just saw this bug / CL. Thanks so much for looking into it.

I'm not sure it's likely to be related to the observed crash but it does appear
to be a bug.

I wonder if the fix is correct though? The fast path does not take a lock so
couldn't CPU instruction re-ordering still cause s_initialized == true to be
seen before the partition is set up? Do we need to do an explicit memory barrier
after initializing the partition to make sure the s_initialized = true; write is
seen only after the partition is initialized?

Tom is stronger than myself on concurrency; cc:ed him for second opinion.