Bug 61145 – [chromium] initialize SchemeRegistry from WebKit::initialize

• Component: New Bugs
• Status: RESOLVED WONTFIX

[chromium] initialize SchemeRegistry from WebKit::initialize

Created attachment 94118 [details]
Patch

If we access WebSecurityOrigin from background threads, we might end up with initializing the SchemeRegistry while accessing it from another thread: See http://crbug.com/71786 for a tsan report

An alternative would be to invoke a WebSecurityOrigin right after calling WebKit::initialize (see http://codereview.chromium.org/7046013/)

wdyt

[Michael Nordman]

Either way works, but i think having it baked into WebKit::initialize is nicer.

Created attachment 94130 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 94130 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94130&action=review

> Source/WebCore/platform/SchemeRegistry.h:42
> +    static void initialize();

maybe this should be named better?

initializeDefaultNoAccessSchemes
ensureDefaultNoAccessSchemesAreInitialized

why do we only need to worry about "no access" schemes?  what about the local ones?

Created attachment 94248 [details]
Patch

(In reply to comment #5)
> (From update of attachment 94130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94130&action=review
> 
> > Source/WebCore/platform/SchemeRegistry.h:42
> > +    static void initialize();
> 
> maybe this should be named better?
> 
> initializeDefaultNoAccessSchemes
> ensureDefaultNoAccessSchemesAreInitialized
> 
> why do we only need to worry about "no access" schemes?  what about the local ones?

I renamed it ensureDefaultSchemesAreInitialized and invoke all static methods that have local static maps

[Darin Fisher (:fishd, Google)]

Comment on attachment 94248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94248&action=review

> Source/WebCore/platform/SchemeRegistry.h:39
> +    // Initialize the default list of schemes. Initialization is optional and

actually, i just realized a potential problem.  using these maps from a background
thread could be problematic if the foreground thread is mutating the maps, right?
there doesn't seem to be any protection for that.

can you say more about how we end up in a situation where we are accessing these
maps off of the main thread?

[Michael Nordman]

(In reply to comment #8)
> (From update of attachment 94248 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94248&action=review
> 
> > Source/WebCore/platform/SchemeRegistry.h:39
> > +    // Initialize the default list of schemes. Initialization is optional and
> 
> actually, i just realized a potential problem.  using these maps from a background
> thread could be problematic if the foreground thread is mutating the maps, right?
> there doesn't seem to be any protection for that.
> 
> can you say more about how we end up in a situation where we are accessing these
> maps off of the main thread?

WebCore::SecurityOrigin uses SchemeRegistry. There are other maps withing SecurityOrigin.cpp that are also not thread-safe.

DEFINE_STATIC_LOCAL(OriginAccessMap, originAccessMap, ());
DEFINE_STATIC_LOCAL(URLSchemesMap, schemes, ());

[Darin Fisher (:fishd, Google)]

maybe the answer is that these maps should only be mutable during app startup.  after that point, it becomes safe to mutate them on background threads.  if we are already accessing them on background threads and not observing a problem, then this is probably why.  maybe this should be made more clear though, and it should probably be enforced with assertions at least.

[Darin Fisher (:fishd, Google)]

(In reply to comment #10)
> maybe the answer is that these maps should only be mutable during app startup.  after that point, it becomes safe to mutate them on background threads.  if we are already accessing them on background threads and not observing a problem, then this is probably why.  maybe this should be made more clear though, and it should probably be enforced with assertions at least.

oops, i meant: "after that point, it becomes safe to [read] them on background threads."

[Michael Nordman]

(In reply to comment #11)
> (In reply to comment #10)
> > maybe the answer is that these maps should only be mutable during app startup.  after that point, it becomes safe to mutate them on background threads.  if we are already accessing them on background threads and not observing a problem, then this is probably why.  maybe this should be made more clear though, and it should probably be enforced with assertions at least.
> 
> oops, i meant: "after that point, it becomes safe to [read] them on background threads."

I'm not familiar with how or why the underlying maps mutate. It could be that we've been getting lucky. Just noticed another one of these static maps hidden away... no clue what it means for a domain to be forbidden from relaxation for a particular scheme. A particular use case that needs to be thoroughly thread-safe is producing and parsing originIdentifier strings so that we can covert to and from urls strings. I think if you ask SecurityOrigin to do one such conversion all of the static tables for that purpose uses will have been initted, but I don't know if any of those particular tables can mutate (like this forbidden relaxation thing).

Maybe the methods that rely on mutating unguarded static maps should assert main thread only?

static HashSet<String>& schemesForbiddenFromDomainRelaxation()
{
    DEFINE_STATIC_LOCAL(HashSet<String>, schemes, ());
    return schemes;
}

void SecurityOrigin::setDomainRelaxationForbiddenForURLScheme(bool forbidden, const String& scheme)
bool SecurityOrigin::isDomainRelaxationForbiddenForURLScheme(const String& scheme)

[Sam Weinig]

(In reply to comment #11)
> (In reply to comment #10)
> > maybe the answer is that these maps should only be mutable during app startup.  after that point, it becomes safe to mutate them on background threads.  if we are already accessing them on background threads and not observing a problem, then this is probably why.  maybe this should be made more clear though, and it should probably be enforced with assertions at least.
> 
> oops, i meant: "after that point, it becomes safe to [read] them on background threads."

I think there is API (or maybe just SPI) exposed that allows mutation of these scheme maps at runtime, so I am not sure that would work.

(In reply to comment #10)
> maybe the answer is that these maps should only be mutable during app startup.  after that point, it becomes safe to mutate them on background threads.  if we are already accessing them on background threads and not observing a problem, then this is probably why.  maybe this should be made more clear though, and it should probably be enforced with assertions at least.


oh, we _are_ observing problems. In our in-process copy of WebKit, we don't properly initialize these maps, so we end up getting races

(In reply to comment #12)
> I'm not familiar with how or why the underlying maps mutate. It could be that we've been getting lucky. 

some of the maps are populated with default entries when they're first accessed. When we access security origins from different threads, we get a race: one thread reads the map, and it's auto-populated, while the other thread also tries to read the map

[Michael Nordman]

> some of the maps are populated with default entries when they're first accessed. When we access security origins from different threads, we get a race: one thread reads the map, and it's auto-populated, while the other thread also tries to read the map

Maybe we should submit the following until we get this worked out better...
http://codereview.chromium.org/7054002/diff/2001/content/browser/in_process_webkit/webkit_thread.cc

There are a variety of "maps" ultimately utilized by this class, those that mutate w/o any guards around them are just not thread-safe. Only some of these "maps" are actually relevant to the functions we really need to access on the IO (and other threads) within chrome's main process. I think (but haven't confirmed this) that those needed do not mutate after initialized.

Maybe instead of using the WebSecurityOrigin instances (which exposes methods that are inherently not thread-safe) to perform those functions, we add new webkit API to perform exactly the originIdentifier cracking and producing functions we need... say a pair of static WebSecurityOrigin methods... WebKit::init ensures the maps used for those functions are initted.

[Michael Nordman]

These methods look safe.

SecurityOrigin::toString()
SecurityOrigin::databaseIdentifier()

The SecurityOrigin constructor code paths are more questionable...

SchemeRegistry::shouldTreatURLSchemeAsNoAccess()
uses schemesWithUniqueOrigins() which can mutate, but apparently does not in the current code base since registerURLSchemeAsNoAccess() is only called from test_shel_webkit_init.

// in SecurityOrigin.cpp
schemeRequiresAuthority()  // Depends on initialization, but safe thereafter, map does not mutate

// from KURL.h
boo isDefaultPortForProtocol()  // Depends on initialization, but safe thereafter, map does not mutate

closing his one, as I don't intend to continue to work on this for now

[Eric Seidel (no email)]

Comment on attachment 94248 [details]
Patch

Cleared review? from attachment 94248 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).