Fix race condition in parallel font initialization.

Uses optional mutex to guard construction of the singleton, which
initializes the non-threadsafe libfontconfig. Without this change,
Skia's parallel path ops test runner crashes 6/10 and hangs 2/10 on
startup; with this change, 0/10 problems.

BUG=skia:2693
R=mtklein@google.com,bungeman@google.com

Committed: https://skia.googlesource.com/skia/+/df022f5972ae6a2a1d96d15c50eca52cade3abd8

Committed: https://skia.googlesource.com/skia/+/60b08a0adfe73f593af62c8d3f55958438360e1b

Committed: https://skia.googlesource.com/skia/+/e438ddbc7451ec80f953266409fea4695e78c8c2

[tomhudson, 2014-06-26 18:35:36]

PTAL

[mtklein, 2014-06-26 18:59:07]

https://codereview.chromium.org/355573006/diff/1/include/ports/SkFontConfigIn...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/355573006/diff/1/include/ports/SkFontConfigIn...
include/ports/SkFontConfigInterface.h:101: static SkFontConfigInterface*
GetSingletonDirectInterface(SkBaseMutex& mutex);
Would mildly prefer passing this as a pointer.

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
File src/ports/SkFontConfigInterface_android.cpp (right):

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
src/ports/SkFontConfigInterface_android.cpp:159: // Doesn't need passed-in mutex
because getSingletonInterface() uses one
Can we refactor to remove that other mutex and use this one instead?  I'd feel a
lot warmer and fuzzier if all variants worked the same way.

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
File src/ports/SkFontConfigInterface_direct.cpp (right):

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
src/ports/SkFontConfigInterface_direct.cpp:129:
SK_DECLARE_STATIC_LAZY_PTR(SkFontConfigInterfaceDirect, direct);
Given the mutex lock, this can become simply

static SkFontConfigInterfaceDirect* singleton = NULL;
if (singleton == NULL) {
  singleton = SkNEW(SkFontConfigInterfaceDirect);
}
return singleton;

That is, if we're under a mutex, SkLazyPtr isn't needed.

The slightly more performant alternative is to do this:

static bool initialized = false;
static SkFontConfigInteface* singleton = NULL;
SkOnce(&initialized, &mutex, CreateDirect, &singleton);
return singleton;

where CreateDirect is something like

/*static*/
void SkFontConfigInterface::CreateDirect(SkFontConfigInterface** ptr) {
  *ptr = SkNEW(SkFontConfigInterfaceDirect);
}

[tomhudson, 2014-06-26 21:01:52]

Looks safe on win & mac build trybots.

https://codereview.chromium.org/355573006/diff/1/include/ports/SkFontConfigIn...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/355573006/diff/1/include/ports/SkFontConfigIn...
include/ports/SkFontConfigInterface.h:101: static SkFontConfigInterface*
GetSingletonDirectInterface(SkBaseMutex& mutex);
I wish for pointers just to avoid the #include. I'd made it a reference to
guarantee non-NULL, but it looks like SkAutoMutexAcquire has sane NULL-handling,
so...

Done

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
File src/ports/SkFontConfigInterface_android.cpp (right):

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
src/ports/SkFontConfigInterface_android.cpp:159: // Doesn't need passed-in mutex
because getSingletonInterface() uses one
getSingletonInterface() has many (5?) other callers. They don't have access to
the mutex we're passing in here, so I don't see an obvious way to do that.

Now, locking the mutex passed in wouldn't *hurt* here, aside from the minor
performance hit.

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
File src/ports/SkFontConfigInterface_direct.cpp (right):

https://codereview.chromium.org/355573006/diff/1/src/ports/SkFontConfigInterf...
src/ports/SkFontConfigInterface_direct.cpp:129:
SK_DECLARE_STATIC_LAZY_PTR(SkFontConfigInterfaceDirect, direct);
On 2014/06/26 18:59:07, mtklein wrote:
> That is, if we're under a mutex, SkLazyPtr isn't needed.

SK_DECLARE_STATIC_LAZY_PTR does nicely describe what we're doing.
I didn't want to introduce the extra CreateDirect() function, so implemented
your slightly-less-performant version.

Done

[mtklein, 2014-06-26 21:05:31]

LGTM

https://codereview.chromium.org/355573006/diff/1/include/ports/SkFontConfigIn...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/355573006/diff/1/include/ports/SkFontConfigIn...
include/ports/SkFontConfigInterface.h:101: static SkFontConfigInterface*
GetSingletonDirectInterface(SkBaseMutex& mutex);
On 2014/06/26 21:01:52, tomhudson wrote:
> I wish for pointers just to avoid the #include.

Just FYI, you can forward declare types used as references too.

[tomhudson, 2014-06-26 21:29:47]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-06-26 21:29:57]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/355573006/40001

[commit-bot: I haz the power, 2014-06-26 21:30:00]

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

[commit-bot: I haz the power, 2014-06-26 21:30:00]

Presubmit check for 355573006-40001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

[tomhudson, 2014-06-27 12:42:17]

+Mike for public API change (to SkFontConfigInterface).

Related: OtherMike: *why* did you want the SkBaseMutex to be passed by pointer
instead of reference?

[mtklein, 2014-06-27 13:41:45]

> Related: OtherMike: *why* did you want the SkBaseMutex to be passed by pointer
> instead of reference?

Just the usual reason we try to pass mutable things as pointers: the mutability
is visible at the call site.  I admit for a mutex it doesn't much matter, as
mutexes really _only_ can be mutated, but it's nice keep it dumb rather than
something you have to think about.

[reed1, 2014-06-30 20:57:24]

lgtm

[tomhudson, 2014-06-30 20:57:35]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-06-30 20:58:31]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/355573006/40001

[commit-bot: I haz the power, 2014-06-30 21:14:04]

Change committed as df022f5972ae6a2a1d96d15c50eca52cade3abd8

[reed2, 2014-06-30 23:47:47]

A revert of this CL has been created in
https://codereview.chromium.org/365503003/ by reed@chromium.org.

The reason for reverting is: breaks chrome builds

.

[mtklein, 2014-07-01 00:14:22]

On 2014/06/30 23:47:47, reed2 wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/365503003/ by mailto:reed@chromium.org.
> 
> The reason for reverting is: breaks chrome builds
> 
> .

Hey now!  That's my button.

Tom was fixing here: https://codereview.chromium.org/364473002/

[tomhudson, 2014-07-01 15:04:56]

On 2014/07/01 00:14:22, mtklein wrote:
> Hey now!  That's my button.
> 
> Tom was fixing here: https://codereview.chromium.org/364473002/

Fix has landed, and build succeeded on the canary, so attempting to re-land
this.

[tomhudson, 2014-07-01 15:05:01]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-07-01 15:06:09]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/355573006/40001

[commit-bot: I haz the power, 2014-07-01 15:06:18]

Change committed as 60b08a0adfe73f593af62c8d3f55958438360e1b

[mtklein, 2014-07-01 20:32:49]

A revert of this CL has been created in
https://codereview.chromium.org/354133004/ by mtklein@google.com.

The reason for reverting is: canaries still slightly broken.

[Tom Hudson, 2014-07-02 01:41:02]

The CQ bit was checked by tomhudson@chromium.org

[commit-bot: I haz the power, 2014-07-02 01:41:40]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/355573006/60001

[commit-bot: I haz the power, 2014-07-02 01:54:47]

Change committed as e438ddbc7451ec80f953266409fea4695e78c8c2