Make static KURLs thread safe

Make static KURLs thread safe

KURLs aren't really thread safe, because they are generally mutable.
Additionally, unless their String members are static strings
(they aren't), then KURLs are *really* not thread safe.

This CL makes sure that blankUrl() is only used on the main thread,
and removes srcdocUrl() because it is used very infrequently.

To help DomURL which wants about blank URLs off the main thread, I
added a char kAboutBlankUrl[] for use in the KURL constructor.

BUG=689044

[Charlie Harrison, 2017-02-09 05:46:20]

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

[commit-bot: I haz the power, 2017-02-09 05:47:00]

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-02-09 07:16:39]

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

[commit-bot: I haz the power, 2017-02-09 07:16:40]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Harrison, 2017-02-09 13:49:14]

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

[commit-bot: I haz the power, 2017-02-09 13:49:27]

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-02-09 15:30:43]

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

[commit-bot: I haz the power, 2017-02-09 15:30:43]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-09 15:51:27]

Description was changed from

==========
Remove data race in KURL::blankUrl

BUG=689044
==========

to

==========
Remove data race in KURL::blankUrl

KURLs aren't really thread safe, because they are generally mutable.
Additionally, unless their String members are static strings
(they aren't), then KURLs are *really* not thread safe.

This CL makes sure that blankUrl() is only used on the main thread,
and removes srcdocUrl() because it is used very infrequently.

To help DomURL which wants about blank URLs off the main thread, I
added a char kAboutBlankUrl[] for use in the KURL constructor.

This results in only minor cleanups needed, which shouldn't impact

BUG=689044
==========

[Charlie Harrison, 2017-02-09 15:52:08]

Description was changed from

==========
Remove data race in KURL::blankUrl

KURLs aren't really thread safe, because they are generally mutable.
Additionally, unless their String members are static strings
(they aren't), then KURLs are *really* not thread safe.

This CL makes sure that blankUrl() is only used on the main thread,
and removes srcdocUrl() because it is used very infrequently.

To help DomURL which wants about blank URLs off the main thread, I
added a char kAboutBlankUrl[] for use in the KURL constructor.

This results in only minor cleanups needed, which shouldn't impact

BUG=689044
==========

to

==========
Make static KURLs thread safe

KURLs aren't really thread safe, because they are generally mutable.
Additionally, unless their String members are static strings
(they aren't), then KURLs are *really* not thread safe.

This CL makes sure that blankUrl() is only used on the main thread,
and removes srcdocUrl() because it is used very infrequently.

To help DomURL which wants about blank URLs off the main thread, I
added a char kAboutBlankUrl[] for use in the KURL constructor.

This results in only minor cleanups needed, which shouldn't impact

BUG=689044
==========

[Charlie Harrison, 2017-02-09 15:56:27]

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

[Charlie Harrison, 2017-02-09 15:56:27]

esprehn: PTAL?

As far as I can tell, this does make things slower, but not
in any bottlenecks:

1. CanvasImageSource::sourceURL: this now has an extra KURL copy, ref bumping
the strings and copying the url::Parsed. For the classes that don't override
sourceURL it results in an extra KURL() construction.

2. DOMURL now constructs "about:blank" for URL resolution at construction and at
setInput.

LMK if you see a way to make this better, or if you'd like another approach
(e.g. THREAD_SAFE_STATIC_LOCAL)

[Charlie Harrison, 2017-02-09 15:56:57]

Description was changed from

==========
Make static KURLs thread safe

KURLs aren't really thread safe, because they are generally mutable.
Additionally, unless their String members are static strings
(they aren't), then KURLs are *really* not thread safe.

This CL makes sure that blankUrl() is only used on the main thread,
and removes srcdocUrl() because it is used very infrequently.

To help DomURL which wants about blank URLs off the main thread, I
added a char kAboutBlankUrl[] for use in the KURL constructor.

This results in only minor cleanups needed, which shouldn't impact

BUG=689044
==========

to

==========
Make static KURLs thread safe

KURLs aren't really thread safe, because they are generally mutable.
Additionally, unless their String members are static strings
(they aren't), then KURLs are *really* not thread safe.

This CL makes sure that blankUrl() is only used on the main thread,
and removes srcdocUrl() because it is used very infrequently.

To help DomURL which wants about blank URLs off the main thread, I
added a char kAboutBlankUrl[] for use in the KURL constructor.

BUG=689044
==========

[esprehn, 2017-02-15 02:05:17]

Why aren't the KURLs constant? a const KURL& should be logically immutable just
like a StringImpl is.

https://codereview.chromium.org/2680933005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMURL.h (right):

https://codereview.chromium.org/2680933005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURL.h:51: return new DOMURL(url,
KURL(ParsedURLString, kAboutBlankUrl),
it seems pretty bad to parse about:blank inside every single URL object
construction.

[Charlie Harrison, 2017-02-15 02:44:03]

Oh yeah I think a const KURL& is constant, but will hold reference to an
internal String (usually an atomic string), which makes them thread-unsafe.
Scratch the comment in the description about them being generally mutable.

This code is just lucky to have not triggered our thread restriction verified,
because KURLs operator== internally uses WTF::equal, which pulls the impls out
without refing them.

https://codereview.chromium.org/2680933005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/DOMURL.h (right):

https://codereview.chromium.org/2680933005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/DOMURL.h:51: return new DOMURL(url,
KURL(ParsedURLString, kAboutBlankUrl),
On 2017/02/15 02:05:17, esprehn wrote:
> it seems pretty bad to parse about:blank inside every single URL object
> construction.

OK, I will think of a redesign.

[esprehn, 2017-02-16 05:51:49]

I see, we should make the static KURLs all be constructed with static StringImpl
instances too, that solves this.

[Charlie Harrison, 2017-02-16 12:56:18]

On 2017/02/16 05:51:49, esprehn wrote:
> I see, we should make the static KURLs all be constructed with static
StringImpl
> instances too, that solves this.

Ok. Before I considered that solution not worth the complexity, but I can
re-think if you prefer it.

[esprehn, 2017-04-07 10:28:27]

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

[esprehn, 2017-04-07 10:28:41]

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