Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2474303004:
Remove mutex locks in SchemeRegistry (Closed)
|
Created:
4 years, 1 month ago by Charlie Harrison Modified:
4 years, 1 month ago CC:
blink-reviews, chromium-reviews, brucedawson Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove mutex locks in SchemeRegistry
This patch removes locks from the SchemeRegistry, in favor of moving all
initialization to the render thread initialization period.
BUG=348655
Committed: https://crrev.com/087c8b21705dd47c9a71058bf743147324425585
Cr-Commit-Position: refs/heads/master@{#431468}
Patch Set 1 #Patch Set 2 : #include #
Total comments: 4
Patch Set 3 : let's try... just exposing the headers? #Patch Set 4 : tkent review #
Total comments: 4
Patch Set 5 : kinuko@ review #
Messages
Total messages: 62 (36 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
csharrison@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@, PTAL? I tried to do as you suggested in the platform-architecture-dev@ thread. Looks like unit tests are fine with these checks. Do you have ideas why the chromeos builds are failing? Somehow they aren't finding WTF::isBeforeThreadCreated which is only compiled #if ENABLED(ASSERT). I thought that would map exactly to DCHECK, but I could try uploading a PS changing the DCHECKs to ASSERTs.
https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:92: DCHECK(WTF::isBeforeThreadCreated()); Given that we're deprecating ASSERT could we change WTF::isBeforeThreadCreated() to be defined if ENABLE(ASSERT) || DCHECK_IS_ON()? (Or only when DCHECK_IS_ON and change callsites to only use DCHECK) https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:165: void SchemeRegistry::initialize() { There're a lot :( Should we quickly check if it doesn't add much initialization cost? I feel like cleaning up these a little further, but for now let's focus on one thing in this patch...
https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:165: void SchemeRegistry::initialize() { On 2016/11/06 11:14:25, kinuko wrote: > There're a lot :( Should we quickly check if it doesn't add much initialization > cost? > > I feel like cleaning up these a little further, but for now let's focus on one > thing in this patch... I locally added a SCOPED_BLINK_UMA_HISTOGRAM_TIMER to time this method on a Nexus 7. It takes on average 40-50 us on renderer startup. Is that acceptable?
https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:92: DCHECK(WTF::isBeforeThreadCreated()); On 2016/11/06 11:14:25, kinuko wrote: > Given that we're deprecating ASSERT could we change WTF::isBeforeThreadCreated() > to be defined if ENABLE(ASSERT) || DCHECK_IS_ON()? > > (Or only when DCHECK_IS_ON and change callsites to only use DCHECK) Unfortunately, it seems like DCHECK requires its arguments to exist during compilation. Your suggestions hit the same problems :/ We could just stop gating isBeforeThreadCreated() and friends. It's only a few methods and a single global bool... WDYT?
On 2016/11/07 17:57:48, Charlie Harrison wrote: > https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): > > https://codereview.chromium.org/2474303004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:92: > DCHECK(WTF::isBeforeThreadCreated()); > On 2016/11/06 11:14:25, kinuko wrote: > > Given that we're deprecating ASSERT could we change > WTF::isBeforeThreadCreated() > > to be defined if ENABLE(ASSERT) || DCHECK_IS_ON()? > > > > (Or only when DCHECK_IS_ON and change callsites to only use DCHECK) > > Unfortunately, it seems like DCHECK requires its arguments to exist during > compilation. Your suggestions hit the same problems :/ We could just stop gating > isBeforeThreadCreated() and friends. It's only a few methods and a single global > bool... WDYT? Do you mean always having it defined regardless of the flag (but let it have empty body if dcheck is not on)? Assuming that it should be optimized way in production build I'm supportive of doing so.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I ended up just exposing the headers unconditionally, where the impl is still gated behind #if ENABLED(ASSERT). What do you think?
Description was changed from ========== Remove mutex locks in SchemeRegistry This patch removes locks from the SchemeRegistry, in favor of moving all initialization to the render thread initialization period. BUG=348655 ========== to ========== Remove mutex locks in SchemeRegistry This patch removes locks from the SchemeRegistry, in favor of moving all initialization to the render thread initialization period. BUG=348655 ==========
kinuko@chromium.org changed reviewers: + tkent@chromium.org
On 2016/11/08 03:44:23, Charlie Harrison wrote: > I ended up just exposing the headers unconditionally, where the impl is still > gated behind #if ENABLED(ASSERT). What do you think? I think this looks okay. +tkent@ for ASSERT related wtf/ changes-- wdyt?
On 2016/11/08 at 04:10:27, kinuko wrote: > On 2016/11/08 03:44:23, Charlie Harrison wrote: > > I ended up just exposing the headers unconditionally, where the impl is still > > gated behind #if ENABLED(ASSERT). What do you think? > > I think this looks okay. > > +tkent@ for ASSERT related wtf/ changes-- wdyt? That's ok. It's too noisy to wrap every DCHECK with #if ENABLE(ASSERT) or DCHECK_IS_ON().
On 2016/11/08 at 05:48:39, tkent wrote: > On 2016/11/08 at 04:10:27, kinuko wrote: > > On 2016/11/08 03:44:23, Charlie Harrison wrote: > > > I ended up just exposing the headers unconditionally, where the impl is still > > > gated behind #if ENABLED(ASSERT). What do you think? > > > > I think this looks okay. > > > > +tkent@ for ASSERT related wtf/ changes-- wdyt? > > That's ok. It's too noisy to wrap every DCHECK with #if ENABLE(ASSERT) or DCHECK_IS_ON(). Keeping ENABLED(ASSERT) in .cpp would cause troubles. We should add a comment about it, or should provide empty implementations in !ENABLE(ASSERT).
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I've added empty impls in the pthreads/winthreads files, and added a comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
scheme registry changes lgtm, I'd like to land this and wait for a few days to see if we have any reports for unexpected DCHECK hits before further cleanups https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:157: void SchemeRegistry::initialize() { Can we add comments about why we need this initialization. Also let's document that new maps need to be added here https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Threading.h (right): https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Threading.h:69: // they can be used within DCHECKs. The empty impls don't really make sense, I might just add a comment to note that they are not defined in !ENABLE(ASSERT) cases and they can only be used inside ASSERT or DCHECKs. (I defer the decisions to Kent-san)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tkent@, ptal? https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right): https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:157: void SchemeRegistry::initialize() { On 2016/11/09 03:56:37, kinuko wrote: > Can we add comments about why we need this initialization. Also let's document > that new maps need to be added here Done. https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Threading.h (right): https://codereview.chromium.org/2474303004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Threading.h:69: // they can be used within DCHECKs. On 2016/11/09 03:56:37, kinuko wrote: > The empty impls don't really make sense, I might just add a comment to note that > they are not defined in !ENABLE(ASSERT) cases and they can only be used inside > ASSERT or DCHECKs. > > (I defer the decisions to Kent-san) I agree with you. Changed to be a comment.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
lgtm
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2474303004/#ps80001 (title: "kinuko@ review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:
While running git apply --index -p1;
error: patch failed:
third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:27
error: third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp: patch
does not apply
Patch: third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
Index: third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
diff --git a/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
b/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
index
ab648fe5e365012856642d61abce8350a35eecf8..89aa5e93407a2b1792a1064f119f9381e582bce2
100644
--- a/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
+++ b/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
@@ -27,27 +27,14 @@
#include "platform/weborigin/SchemeRegistry.h"
#include "wtf/ThreadSpecific.h"
+#include "wtf/Threading.h"
#include "wtf/ThreadingPrimitives.h"
#include "wtf/text/StringBuilder.h"
namespace blink {
-static Mutex& mutex() {
- // The first call to this should be made before or during blink
- // initialization to avoid racy static local initialization.
- DEFINE_STATIC_LOCAL(Mutex, m, ());
- return m;
-}
-
-// Defines static local variable after making sure that a lock is held.
-// (We can't use DEFINE_STATIC_LOCAL for this because it asserts thread
-// safety, which is externally guaranteed by the local mutex() lock)
-#define DEFINE_STATIC_LOCAL_WITH_LOCK(type, name, arguments) \
- ASSERT(mutex().locked()); \
- static type& name = *new type arguments
-
static URLSchemesSet& localURLSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, localSchemes, ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, localSchemes, ());
if (localSchemes.isEmpty())
localSchemes.add("file");
@@ -56,58 +43,55 @@ static URLSchemesSet& localURLSchemes() {
}
static URLSchemesSet& displayIsolatedURLSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, displayIsolatedSchemes, ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, displayIsolatedSchemes, ());
return displayIsolatedSchemes;
}
static URLSchemesSet& secureSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, secureSchemes,
- ({
- "https", "about", "data", "wss",
- }));
+ DEFINE_STATIC_LOCAL(URLSchemesSet, secureSchemes,
+ ({
+ "https", "about", "data", "wss",
+ }));
return secureSchemes;
}
static URLSchemesSet& schemesWithUniqueOrigins() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, schemesWithUniqueOrigins,
- ({
- "about", "javascript", "data",
- }));
+ DEFINE_STATIC_LOCAL(URLSchemesSet, schemesWithUniqueOrigins,
+ ({
+ "about", "javascript", "data",
+ }));
return schemesWithUniqueOrigins;
}
static URLSchemesSet& emptyDocumentSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, emptyDocumentSchemes,
- ({
- "about",
- }));
+ DEFINE_STATIC_LOCAL(URLSchemesSet, emptyDocumentSchemes, ({
+ "about",
+ }));
return emptyDocumentSchemes;
}
static HashSet<String>& schemesForbiddenFromDomainRelaxation() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(HashSet<String>, schemes, ());
+ DEFINE_STATIC_LOCAL(HashSet<String>, schemes, ());
return schemes;
}
static URLSchemesSet& notAllowingJavascriptURLsSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet,
notAllowingJavascriptURLsSchemes,
- ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, notAllowingJavascriptURLsSchemes, ());
return notAllowingJavascriptURLsSchemes;
}
void SchemeRegistry::registerURLSchemeAsLocal(const String& scheme) {
+ DCHECK(WTF::isBeforeThreadCreated());
DCHECK_EQ(scheme, scheme.lower());
- MutexLocker locker(mutex());
localURLSchemes().add(scheme);
}
const URLSchemesSet& SchemeRegistry::localSchemes() {
- MutexLocker locker(mutex());
return localURLSchemes();
}
static URLSchemesSet& CORSEnabledSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, CORSEnabledSchemes, ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, CORSEnabledSchemes, ());
if (CORSEnabledSchemes.isEmpty()) {
CORSEnabledSchemes.add("http");
@@ -119,7 +103,7 @@ static URLSchemesSet& CORSEnabledSchemes() {
}
static URLSchemesSet& serviceWorkerSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, serviceWorkerSchemes, ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, serviceWorkerSchemes, ());
if (serviceWorkerSchemes.isEmpty()) {
// HTTP is required because http://localhost is considered secure.
@@ -133,7 +117,7 @@ static URLSchemesSet& serviceWorkerSchemes() {
}
static URLSchemesSet& fetchAPISchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, fetchAPISchemes, ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, fetchAPISchemes, ());
if (fetchAPISchemes.isEmpty()) {
fetchAPISchemes.add("http");
@@ -144,26 +128,23 @@ static URLSchemesSet& fetchAPISchemes() {
}
static URLSchemesSet& firstPartyWhenTopLevelSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, firstPartyWhenTopLevelSchemes,
- ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, firstPartyWhenTopLevelSchemes, ());
return firstPartyWhenTopLevelSchemes;
}
static URLSchemesMap<SchemeRegistry::PolicyAreas>&
ContentSecurityPolicyBypassingSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesMap<SchemeRegistry::PolicyAreas>,
- schemes, ());
+ DEFINE_STATIC_LOCAL(URLSchemesMap<SchemeRegistry::PolicyAreas>, schemes, ());
return schemes;
}
static URLSchemesSet& secureContextBypassingSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, secureContextBypassingSchemes,
- ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, secureContextBypassingSchemes, ());
return secureContextBypassingSchemes;
}
static URLSchemesSet& allowedInReferrerSchemes() {
- DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, allowedInReferrerSchemes, ());
+ DEFINE_STATIC_LOCAL(URLSchemesSet, allowedInReferrerSchemes, ());
if (allowedInReferrerSchemes.isEmpty()) {
allowedInReferrerSchemes.add("http");
@@ -173,22 +154,35 @@ static URLSchemesSet& allowedInReferrerSchemes() {
return allowedInReferrerSchemes;
}
+// All new maps should be added here. Must be called before we create other
+// threads to avoid racy static local initialization.
void SchemeRegistry::initialize() {
- // Instantiate the mutex object.
- mutex();
+ localURLSchemes();
+ displayIsolatedURLSchemes();
+ secureSchemes();
+ schemesWithUniqueOrigins();
+ emptyDocumentSchemes();
+ schemesForbiddenFromDomainRelaxation();
+ notAllowingJavascriptURLsSchemes();
+ CORSEnabledSchemes();
+ serviceWorkerSchemes();
+ fetchAPISchemes();
+ firstPartyWhenTopLevelSchemes();
+ ContentSecurityPolicyBypassingSchemes();
+ secureContextBypassingSchemes();
+ allowedInReferrerSchemes();
}
bool SchemeRegistry::shouldTreatURLSchemeAsLocal(const String& scheme) {
DCHECK_EQ(scheme, scheme.lower());
if (scheme.isEmpty())
return false;
- MutexLocker locker(mutex());
return localURLSchemes().contains(scheme);
}
void SchemeRegistry::registerURLSchemeAsNoAccess(const String& scheme) {
+ DCHECK(WTF::isBeforeThreadCreated());
DCHECK_EQ(scheme, scheme.lower());
- MutexLocker locker(mutex());
schemesWithUniqueOrigins().add(scheme);
}
@@ -196,13 +190,12 @@ bool SchemeRegistry::shouldTreatURLSchemeAsNoAccess(const
String& scheme) {
DCHECK_EQ(scheme, scheme.lower());
if (scheme.isEmpty())
return false;
- MutexLocker locker(mutex());
return schemesWithUniqueOrigins().contains(scheme);
}
void SchemeRegistry::registerURLSchemeAsDisplayIsolated(const String& scheme) {
+ DCHECK(WTF::isBeforeThreadCreated());
DCHECK_EQ(scheme, scheme.lower());
- MutexLocker locker(mutex());
displayIsolatedURLSchemes().add(scheme);
}
@@ -211,7 +204,6 @@ bool SchemeRegistry::shouldTreatURLSchemeAsDisplayIsolated(
DCHECK_EQ(scheme, scheme.lower());
if (scheme.isEmpty())
return false;
- MutexLocker locker(mutex());
return displayIsolatedURLSchemes().contains(scheme);
}
@@ -222,8 +214,8 @@ bool
SchemeRegistry::shouldTreatURLSchemeAsRestrictingMixedContent(
}
void SchemeRegistry::registerURLSchemeAsSecure(const String& scheme) {
+ DCHECK(WTF::isBeforeThreadCreated());
DCHECK_EQ(scheme, scheme.lower());
- MutexLocker locker(mutex());
secureSchemes().add(scheme);
}
@@ -231,13 +223,12 @@ bool SchemeRegistry::shouldTreatURLSchemeAsSecure(const
String& scheme) {
DCHECK_EQ(scheme, scheme.lower());
if (scheme.isEmpty())
return false;
- MutexLocker locker(mutex());
return secureSchemes().contains(scheme);
}
void SchemeRegistry::registerURLSchemeAsEmptyDocument(const String& scheme) {
+ DCHECK(WTF::isBeforeThreadCreated());
DCHECK_EQ(scheme, scheme.lower());
- MutexLocker locker(mutex());
emptyDocumentSchemes().add(scheme);
}
@@ -245,18 +236,17 @@ bool
SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument(const String& scheme) {
DCHECK_EQ(scheme, scheme.lower());
if (scheme.isEmpty())
return false;
- MutexLocker locker(mutex());
return emptyDocumentSchemes().contains(scheme);
}
void SchemeRegistry::setDomainRelaxationForbiddenForURLScheme(
bool forbidden,
const String& scheme) {
+ DCHECK(WTF::isBeforeThreadCreated());
DCHECK_EQ(scheme, scheme.lower());
if (sc…
(message too large)
Message was sent while issue was closed.
Description was changed from ========== Remove mutex locks in SchemeRegistry This patch removes locks from the SchemeRegistry, in favor of moving all initialization to the render thread initialization period. BUG=348655 ========== to ========== Remove mutex locks in SchemeRegistry This patch removes locks from the SchemeRegistry, in favor of moving all initialization to the render thread initialization period. BUG=348655 Committed: https://crrev.com/087c8b21705dd47c9a71058bf743147324425585 Cr-Commit-Position: refs/heads/master@{#431468} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/087c8b21705dd47c9a71058bf743147324425585 Cr-Commit-Position: refs/heads/master@{#431468}
Message was sent while issue was closed.
sebmarchand@chromium.org changed reviewers: + sebmarchand@chromium.org
Message was sent while issue was closed.
For some reason this is breaking the official Windows PGO build: https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20PGO%20Bui... It looks like a configuration issue in the BUILD.gn files, but I'm not sure what the problem is yet. Could you please revert this CL while I'm looking at the issue?
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2498623002/ by csharrison@chromium.org. The reason for reverting is: Causing build issues https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Win%20PGO%20Bui....
Message was sent while issue was closed.
I guess PGO builds hate this. Let me see if I can refactor so we don't need compile guards. Sébastien: is this the win-pgo try-bot?
Message was sent while issue was closed.
On 2016/11/11 21:57:23, Charlie Harrison wrote: > I guess PGO builds hate this. Let me see if I can refactor so we don't need > compile guards. > > Sébastien: is this the win-pgo try-bot? It's not just the PGO builds, it's also the LTCG builds. You can repro this locally by setting the following GN flags: is_official_build = true is_debug = false full_wpo_on_official = true I've been able to repro the same issue. I wanted to look at this today but I've been busy with a release blocker. The win_pgo trybot broke earlier this week (I need to look at this next week...)
Message was sent while issue was closed.
On 2016/11/11 22:00:38, Sébastien Marchand wrote: > On 2016/11/11 21:57:23, Charlie Harrison wrote: > > I guess PGO builds hate this. Let me see if I can refactor so we don't need > > compile guards. > > > > Sébastien: is this the win-pgo try-bot? > > It's not just the PGO builds, it's also the LTCG builds. You can repro this > locally by setting the following GN flags: > > is_official_build = true > is_debug = false > full_wpo_on_official = true > > I've been able to repro the same issue. I wanted to look at this today but I've > been busy with a release blocker. > > The win_pgo trybot broke earlier this week (I need to look at this next week...) Well the revert landed so hopefully at least this issue will be fixed. I'll try to make sure we can build with those flags before re-landing. Thanks!
Message was sent while issue was closed.
The implementation of the 'isBeforeThreadCreated' function is encapsulated in a '#if ENABLE(ASSERT)' block, is it possible that this isn't evaluating to true in an official build?
Message was sent while issue was closed.
On 2016/11/11 22:19:08, Sébastien Marchand wrote: > The implementation of the 'isBeforeThreadCreated' function is encapsulated in a > '#if ENABLE(ASSERT)' block, is it possible that this isn't evaluating to true in > an official build? Yes. I'm thinking of just putting the bodies in DCHECKs instead of ifdefing the implementations out.
Message was sent while issue was closed.
Yeah, I've confirmed that this is the problem, not sure why though.
Message was sent while issue was closed.
+1, this isn't the first time that I've seen this kind of issue. IMHO if a function implementation is behind to a preprocessor flag then its declaration should also be behind this same flag.
Message was sent while issue was closed.
On 2016/11/11 22:23:25, Sébastien Marchand wrote: > +1, this isn't the first time that I've seen this kind of issue. > > IMHO if a function implementation is behind to a preprocessor flag then its > declaration should also be behind this same flag. Thanks, good rule of thumb. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
