Stop using DEFINE_STATIC_LOCAL in ResourceResponse.cpp.

Stop using DEFINE_STATIC_LOCAL in ResourceResponse.cpp.

There are several AtomicStrings defined using DEFINE_STATIC_LOCAL in ResourceResponse.cpp.
When those strings are touched on the worker thread, the ThreadRestrictionVerifier check in StringImpl.h fails.
https://crbug.com/681581#c5

To avoid this check failure, this CL makes those strings static.

BUG=681581
Review-Url: https://codereview.chromium.org/2638623004
Cr-Commit-Position: refs/heads/master@{#444016}
Committed: https://chromium.googlesource.com/chromium/src/+/ad218510d6560dbc2b87d878f4e8b2ed5ac9730a

[horo, 2017-01-17 00:34:28]

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

[commit-bot: I haz the power, 2017-01-17 00:34:35]

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

[commit-bot: I haz the power, 2017-01-17 00:43:33]

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

[commit-bot: I haz the power, 2017-01-17 00:43:33]

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

[horo, 2017-01-17 01:45:56]

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

[commit-bot: I haz the power, 2017-01-17 01:46:07]

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

[horo, 2017-01-17 02:17:53]

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

[commit-bot: I haz the power, 2017-01-17 02:18:04]

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

[horo, 2017-01-17 02:18:08]

Patchset #2 (id:20001) has been deleted

[horo, 2017-01-17 02:18:10]

Patchset #1 (id:1) has been deleted

[horo, 2017-01-17 02:19:02]

Description was changed from

==========
use static atomicstring

BUG=
==========

to

==========
Stop using DEFINE_STATIC_LOCAL in ResourceResponse.cpp.

There are several AtomicStrings defined using DEFINE_STATIC_LOCAL in
ResourceResponse.cpp.
When those strings are touched on the worker thread, the
ThreadRestrictionVerifier check in StringImpl.h fails.
https://crbug.com/681581#c5

To avoid this check failure, this CL makes those strings static.

BUG=681581
==========

[horo, 2017-01-17 02:20:44]

horo@chromium.org changed reviewers:
+ haraken@chromium.org

[horo, 2017-01-17 02:20:44]

haraken@
Could you please review this?

[Charlie Harrison, 2017-01-17 02:39:31]

csharrison@chromium.org changed reviewers:
+ csharrison@chromium.org

[Charlie Harrison, 2017-01-17 02:39:31]

drive by: would you mind adding a unit test / layout test that triggers the
DCHECK without this patch?

[haraken, 2017-01-17 02:45:36]

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

[haraken, 2017-01-17 02:45:37]

Can we use DEFINE_THREAD_SAFE_STATIC_LOCAL instead?

I'm not sure if these AtomicStrings are common enough to unconditionally
initialize on the main thread.

+esprehn: What's an expected way to instantiate these "static" AtomicStrings?

[commit-bot: I haz the power, 2017-01-17 04:14:36]

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

[commit-bot: I haz the power, 2017-01-17 04:14:37]

Dry run: This issue passed the CQ dry run.

[horo, 2017-01-17 05:46:59]

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

[commit-bot: I haz the power, 2017-01-17 05:47:04]

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

[horo, 2017-01-17 05:48:10]

On 2017/01/17 02:39:31, Charlie Harrison wrote:
> drive by: would you mind adding a unit test / layout test that triggers the
> DCHECK without this patch?

Added unit tests in Patch Set 3.

[commit-bot: I haz the power, 2017-01-17 07:22:09]

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

[commit-bot: I haz the power, 2017-01-17 07:22:09]

Dry run: This issue passed the CQ dry run.

[sof, 2017-01-17 07:29:59]

On 2017/01/17 02:45:37, haraken wrote:
> Can we use DEFINE_THREAD_SAFE_STATIC_LOCAL instead?
> 
> I'm not sure if these AtomicStrings are common enough to unconditionally
> initialize on the main thread.
> 
> +esprehn: What's an expected way to instantiate these "static" AtomicStrings?

Many of these short AtomicStrings are only used as the "rhs" argument to
equalIgnoringCase(), so can't they just be "const char[]"?

(ContentSecurityPolicy follows this tack, as it and its strings are also
accessed by multiple threads.)

[kinuko, 2017-01-17 08:10:39]

On 2017/01/17 07:29:59, sof wrote:
> On 2017/01/17 02:45:37, haraken wrote:
> > Can we use DEFINE_THREAD_SAFE_STATIC_LOCAL instead?
> > 
> > I'm not sure if these AtomicStrings are common enough to unconditionally
> > initialize on the main thread.
> > 
> > +esprehn: What's an expected way to instantiate these "static"
AtomicStrings?
> 
> Many of these short AtomicStrings are only used as the "rhs" argument to
> equalIgnoringCase(), so can't they just be "const char[]"?
> 
> (ContentSecurityPolicy follows this tack, as it and its strings are also
> accessed by multiple threads.)

+1, I don't think we should put all these in WTF static table, string constant
seems enough for most cases.

[esprehn, 2017-01-17 08:23:00]

On 2017/01/17 at 02:45:37, haraken wrote:
> Can we use DEFINE_THREAD_SAFE_STATIC_LOCAL instead?

You can't do that since there's a separate AtomicString table on each thread in
TLS. The thread safe local means you're racing which thread gets to make the
string, and also the equal() optimization which says if two strings are both
isAtomic() then compare the ptr, but given two AtomicStrings from two different
threads that doesn't hold true as theyre separate tables.

> 
> I'm not sure if these AtomicStrings are common enough to unconditionally
initialize on the main thread.
> 
> +esprehn: What's an expected way to instantiate these "static" AtomicStrings?

Same as atoms, you have to declare them static and have them initialized in the
static string table which is copied into every threads atomic string table
solving the issues above.

[esprehn, 2017-01-17 08:28:15]

You can fix this a few ways:

- using a ThreadSpecific<AtomicString>, but now you pay TLS cost on access. I
don't know if this is cheaper than the hash table for small strings.
- using a static string, but now every thread has this extra memory in the
atomic string table at all times.
- using literal strings

You have to pick which one is best for this use case. Literal strings sound like
a good choice if we don't hit this code path frequently and don't want to pay
the cost all the time, the StringView optimization means you don't allocate
temporaries and the length will be computed at compile time.

[horo, 2017-01-17 08:32:43]

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

[commit-bot: I haz the power, 2017-01-17 08:32:51]

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

[horo, 2017-01-17 08:34:33]

On 2017/01/17 08:10:39, kinuko wrote:
> On 2017/01/17 07:29:59, sof wrote:
> > On 2017/01/17 02:45:37, haraken wrote:
> > > Can we use DEFINE_THREAD_SAFE_STATIC_LOCAL instead?
> > > 
> > > I'm not sure if these AtomicStrings are common enough to unconditionally
> > > initialize on the main thread.
> > > 
> > > +esprehn: What's an expected way to instantiate these "static"
> AtomicStrings?
> > 
> > Many of these short AtomicStrings are only used as the "rhs" argument to
> > equalIgnoringCase(), so can't they just be "const char[]"?
> > 
> > (ContentSecurityPolicy follows this tack, as it and its strings are also
> > accessed by multiple threads.)
> 
> +1, I don't think we should put all these in WTF static table, string constant
> seems enough for most cases.

done

[haraken, 2017-01-17 08:49:00]

LGTM

[horo, 2017-01-17 08:54:46]

On 2017/01/17 08:49:00, haraken wrote:
> LGTM

Thank you

[horo, 2017-01-17 08:54:59]

The CQ bit was unchecked by horo@chromium.org

[horo, 2017-01-17 08:55:07]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2017-01-17 08:55:14]

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

[sof, 2017-01-17 09:04:29]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-01-17 09:04:29]

Opting for short literal strings for equalIgnoringCase() uses seems fine, but
could you write out the reasoning for opting for literal strings when doing
HTTPHeaderMap lookups?

[commit-bot: I haz the power, 2017-01-17 09:57:57]

CQ is committing da patch.

Bot data: {"patchset_id": 20002, "attempt_start_ts": 1484643307601250,
"parent_rev": "f4e64bdfdc124cb42fae353801bd02a88f91d52c", "commit_rev":
"ad218510d6560dbc2b87d878f4e8b2ed5ac9730a"}

[commit-bot: I haz the power, 2017-01-17 09:58:28]

Description was changed from

==========
Stop using DEFINE_STATIC_LOCAL in ResourceResponse.cpp.

There are several AtomicStrings defined using DEFINE_STATIC_LOCAL in
ResourceResponse.cpp.
When those strings are touched on the worker thread, the
ThreadRestrictionVerifier check in StringImpl.h fails.
https://crbug.com/681581#c5

To avoid this check failure, this CL makes those strings static.

BUG=681581
==========

to

==========
Stop using DEFINE_STATIC_LOCAL in ResourceResponse.cpp.

There are several AtomicStrings defined using DEFINE_STATIC_LOCAL in
ResourceResponse.cpp.
When those strings are touched on the worker thread, the
ThreadRestrictionVerifier check in StringImpl.h fails.
https://crbug.com/681581#c5

To avoid this check failure, this CL makes those strings static.

BUG=681581

Review-Url: https://codereview.chromium.org/2638623004
Cr-Commit-Position: refs/heads/master@{#444016}
Committed:
https://chromium.googlesource.com/chromium/src/+/ad218510d6560dbc2b87d878f4e8...
==========

[commit-bot: I haz the power, 2017-01-17 09:58:29]

Committed patchset #4 (id:20002) as
https://chromium.googlesource.com/chromium/src/+/ad218510d6560dbc2b87d878f4e8...

[Charlie Harrison, 2017-01-17 13:32:39]

LGTM but by making the header map strings non atomic we are explicitly opting
into re-hashing the string for every lookup (to find it in the atomic string
table). This can be expensive, so if we see it show up in profiles we may want
to re-consider this approach.