Patch forthcoming.
<rdar://problem/36970526>
Created attachment 332502 [details] work in progress
Created attachment 332503 [details] more
Created attachment 332508 [details] passing some tests
Created attachment 332588 [details] the patch
Comment on attachment 332588 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=332588&action=review r=me > Source/JavaScriptCore/heap/BlockDirectory.cpp:97 > + } I don't understand this loop. What changes on each iteration? > Source/JavaScriptCore/heap/MarkedBlock.h:237 > + SecurityOriginToken m_securityOriginToken { 0 }; Is 0 ever a valid token? > Source/JavaScriptCore/heap/SecurityOriginToken.cpp:36 > + return WTF::atomicExchangeAdd(&counter, 1) + 1; Looks like 0 is never valid from this? :-) > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:35 > +RefPtr<ThreadLocalCache> ThreadLocalCache::create(Heap& heap, SecurityOriginToken securityOriginToken) It's weird to pass securityOriginToken in here, since it's a default param in the header, and IIUC it's never anything else. Could you just not pass it here, and give it to the ctor call below instead? > Source/JavaScriptCore/runtime/JSGlobalObject.h:494 > + JS_EXPORT_PRIVATE explicit JSGlobalObject(VM&, Structure*, const GlobalObjectMethodTable* = 0); I love how this used both 0 and nullptr. > Source/JavaScriptCore/runtime/JSGlobalObject.h:900 > + createThreadLocalCache(); This check-and-create isn't racy is it? Seems like it can't if it's a per-thread thing!
(In reply to JF Bastien from comment #6) > Comment on attachment 332588 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332588&action=review > > r=me > > > Source/JavaScriptCore/heap/BlockDirectory.cpp:97 > > + } > > I don't understand this loop. What changes on each iteration? This part: size_t blockIndex = allocator.m_allocationCursor++; > > > Source/JavaScriptCore/heap/MarkedBlock.h:237 > > + SecurityOriginToken m_securityOriginToken { 0 }; > > Is 0 ever a valid token? Yes. From the GC's standpoint, the tokens are just for diversity. 0 could mean you don't care, so you end up in the same pool as everyone else who doesn't care. Though right now, all ThreadLocalCaches use a unique token. > > > Source/JavaScriptCore/heap/SecurityOriginToken.cpp:36 > > + return WTF::atomicExchangeAdd(&counter, 1) + 1; > > Looks like 0 is never valid from this? :-) No, it means that 0 is not equal to any "unique" token. > > > Source/JavaScriptCore/heap/ThreadLocalCache.cpp:35 > > +RefPtr<ThreadLocalCache> ThreadLocalCache::create(Heap& heap, SecurityOriginToken securityOriginToken) > > It's weird to pass securityOriginToken in here, since it's a default param > in the header, and IIUC it's never anything else. Could you just not pass it > here, and give it to the ctor call below instead? I could. This signals the intent that you could select whatever token you like. > > > Source/JavaScriptCore/runtime/JSGlobalObject.h:494 > > + JS_EXPORT_PRIVATE explicit JSGlobalObject(VM&, Structure*, const GlobalObjectMethodTable* = 0); > > I love how this used both 0 and nullptr. > > > Source/JavaScriptCore/runtime/JSGlobalObject.h:900 > > + createThreadLocalCache(); > > This check-and-create isn't racy is it? Seems like it can't if it's a > per-thread thing! It's not racy because JSGlobalObject methods can only be called on the main thread.
I ended up changing the ThreadLocalCache creation logic so that WebCore::SecurityOrigin creates it.
Created attachment 332666 [details] new patch The JSC part was already reviewed by JF, but I introduced some WebCore changes since then. Can a WebCore reviewer take a look?
Attachment 332666 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/ThreadLocalCache.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332666 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=332666&action=review r=me on the WebCore part. > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:60 > + : JSGlobalObject(vm, structure, globalObjectMethodTable, threadLocalCache) I'd WTFMove(threadLocalCache) here.
(In reply to Andy Estes from comment #11) > Comment on attachment 332666 [details] > new patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332666&action=review > > r=me on the WebCore part. > > > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:60 > > + : JSGlobalObject(vm, structure, globalObjectMethodTable, threadLocalCache) > > I'd WTFMove(threadLocalCache) here. Oh right! Thanks!
Comment on attachment 332666 [details] new patch WebCore r=estes, JSC r=jfb.
Comment on attachment 332666 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=332666&action=review > Source/WebCore/page/SecurityOrigin.h:208 > + RefPtr<JSC::ThreadLocalCache> threadLocalCache(); Seems odd to have a ThreadLocalCache per SecurityOrigin object. Those objects are frequently created and we may have a lot of instances actually referring to the same origin.
Comment on attachment 332666 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=332666&action=review > Source/WebCore/page/SecurityOrigin.cpp:605 > +RefPtr<JSC::ThreadLocalCache> SecurityOrigin::threadLocalCache() > +{ > + if (!m_threadLocalCache) > + m_threadLocalCache = JSC::ThreadLocalCache::create(commonVM().heap); > + return m_threadLocalCache; > +} How did you come to the decision to place this logic in SecurityOrigin? I'm unclear how this relates to the concept of a web origin, the tuple (scheme, host, port), and the same-origin policy that embody the purpose of the class SecurityOrigin. This cache seems more relevant to a script execution context as opposed to arbitrary code that instantiates a SecurityOrigin for the purpose of performing same-origin checks. I know that Web Workers do not have access to a DOM window. It seems strange that they would have their own thread local cache with respect to the VM of the document (that created them) as opposed to their own VM (WorkerGlobalScope::vm()). There is a similar weirdness for Service Workers (ServiceWorkerGlobalScope extends WorkerGlobalScope). ScriptExecutionContext/SecurityContext seems like a more appropriate place for this logic.
Comment on attachment 332666 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=332666&action=review >> Source/WebCore/page/SecurityOrigin.h:208 >> + RefPtr<JSC::ThreadLocalCache> threadLocalCache(); > > Seems odd to have a ThreadLocalCache per SecurityOrigin object. Those objects are frequently created and we may have a lot of instances actually referring to the same origin. Phil mentioned offline that SecurityOrigin objects are cached per-thread (see getCachedOrigin). Given this, I think this is fine after all.
(In reply to Daniel Bates from comment #15) > Comment on attachment 332666 [details] > new patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332666&action=review > > > Source/WebCore/page/SecurityOrigin.cpp:605 > > +RefPtr<JSC::ThreadLocalCache> SecurityOrigin::threadLocalCache() > > +{ > > + if (!m_threadLocalCache) > > + m_threadLocalCache = JSC::ThreadLocalCache::create(commonVM().heap); > > + return m_threadLocalCache; > > +} > > How did you come to the decision to place this logic in SecurityOrigin? I'm > unclear how this relates to the concept of a web origin, the tuple (scheme, > host, port), and the same-origin policy that embody the purpose of the class > SecurityOrigin. This cache seems more relevant to a script execution context > as opposed to arbitrary code that instantiates a SecurityOrigin for the > purpose of performing same-origin checks. Doesn't the fact that SecurityOrigin is used for same-origin checks make it exactly the right object for this purpose? Do we create a different ScriptExecutionContext for every origin? > I know that Web Workers do not > have access to a DOM window. It seems strange that they would have their own > thread local cache They don't. > with respect to the VM of the document (that created > them) as opposed to their own VM (WorkerGlobalScope::vm()). There is a > similar weirdness for Service Workers (ServiceWorkerGlobalScope extends > WorkerGlobalScope). ScriptExecutionContext/SecurityContext seems like a more > appropriate place for this logic. If service workers don't extend JSDOMWindowBase, then they will not partake in the TLC shenanigans. (In reply to Chris Dumez from comment #14) > Comment on attachment 332666 [details] > new patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332666&action=review > > > Source/WebCore/page/SecurityOrigin.h:208 > > + RefPtr<JSC::ThreadLocalCache> threadLocalCache(); > > Seems odd to have a ThreadLocalCache per SecurityOrigin object. Those > objects are frequently created and we may have a lot of instances actually > referring to the same origin. Not every SecurityOrigin will have a TLC. Only if a SecurityOrigin is the securtyOrigin of the document that globalObject's create() sees will that SecurityOrigin be asked to create a TLC. We will have at most one TLC per global object. A previous version of this patch created a separate TLC per global object. That was sound, but slower than this. This improved patch creates at most a separate TLC per global object. Empirically this makes the perf cost go away.
More offline discussion revealed that SecurityOrigin is not hash consed. I'm going to keep working on this.
Created attachment 332810 [details] the patch Updated the WebCore par based on Dan's and Chris's feedback. I'm now creating TLCs by hash-consing the relevant data from SecurityOrigin.
Comment on attachment 332810 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=332810&action=review > Source/WebCore/page/OriginThreadLocalCache.cpp:52 > + dataLog(RawPointer(this), ": destroying origin TLC.\n"); I will remove. > Source/WebCore/page/OriginThreadLocalCache.cpp:63 > + dataLog(RawPointer(this), ": creating origin TLC.\n"); I will remove.
Attachment 332810 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/ThreadLocalCache.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 332811 [details] the patch Fixed build
Attachment 332811 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/ThreadLocalCache.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
*** Bug 81578 has been marked as a duplicate of this bug. ***
Comment on attachment 332810 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=332810&action=review I only looked at the WebCore bits and it look good overall (a few comments though). > Source/WebCore/ChangeLog:1 > +2018-01-30 Filip Pizlo <fpizlo@apple.com> Double change log. > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:59 > +JSDOMGlobalObject::JSDOMGlobalObject(VM& vm, Structure* structure, Ref<DOMWrapperWorld>&& world, const GlobalObjectMethodTable* globalObjectMethodTable, RefPtr<JSC::ThreadLocalCache> threadLocalCache) We usually use && for parameters like these. > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:82 > + : JSDOMGlobalObject(vm, structure, proxy->world(), &s_globalObjectMethodTable, window->document()->securityOrigin().threadLocalCache()) The next line makes me worry about window potentially being null? > Source/WebCore/page/OriginThreadLocalCache.cpp:33 > +typedef HashMap<ThreadLocalCacheOriginKey, OriginThreadLocalCache*> ThreadLocalCacheMap; I believe we started transitioning to "using" statements in WebCore: using ThreadLocalCacheMap = HashMap<ThreadLocalCacheOriginKey, OriginThreadLocalCache*>; > Source/WebCore/page/OriginThreadLocalCache.cpp:41 > +RefPtr<OriginThreadLocalCache> OriginThreadLocalCache::create(const ThreadLocalCacheOriginKey& key) Ref<OriginThreadLocalCache> > Source/WebCore/page/OriginThreadLocalCache.cpp:45 > + return iter->value; return *iter->value; > Source/WebCore/page/OriginThreadLocalCache.cpp:47 > + return adoptRef(new OriginThreadLocalCache(key)); return adoptRef(*new OriginThreadLocalCache(key)); > Source/WebCore/page/OriginThreadLocalCache.cpp:52 > + dataLog(RawPointer(this), ": destroying origin TLC.\n"); should probably be removed. > Source/WebCore/page/OriginThreadLocalCache.cpp:62 > + RELEASE_ASSERT(result); result is a AddResult. Did you mean RELEASE_ASSERT(result.isNewEntry) ? > Source/WebCore/page/OriginThreadLocalCache.cpp:63 > + dataLog(RawPointer(this), ": creating origin TLC.\n"); should probably be removed. > Source/WebCore/page/OriginThreadLocalCache.h:30 > +#include <wtf/HashMap.h> Move to cpp? > Source/WebCore/page/OriginThreadLocalCache.h:31 > +#include <wtf/NeverDestroyed.h> Move to cpp? > Source/WebCore/page/OriginThreadLocalCache.h:35 > +class OriginThreadLocalCache : public JSC::ThreadLocalCache { Should this be marked as final? > Source/WebCore/page/OriginThreadLocalCache.h:37 > + static RefPtr<OriginThreadLocalCache> create(const ThreadLocalCacheOriginKey&); This can never return nullptr, right? If so, it should return a Ref<>, not a RefPtr<>. > Source/WebCore/page/OriginThreadLocalCache.h:42 > + OriginThreadLocalCache(const ThreadLocalCacheOriginKey&); explicit? > Source/WebCore/page/SecurityOrigin.cpp:602 > +RefPtr<JSC::ThreadLocalCache> SecurityOrigin::threadLocalCache() We're not transferring ownership to the caller, I believe we would usually return a JSC::ThreadLocalCache& in such cases. > Source/WebCore/page/ThreadLocalCacheOriginKey.h:41 > + ThreadLocalCacheOriginKey(String host, String protocol, std::optional<uint16_t> port) we usually pass strings as "const String&". > Source/WebCore/page/ThreadLocalCacheOriginKey.h:74 > + return m_host.hash() + m_protocol.hash() + (m_port ? *m_port : 0); I guess this is fine but I think we have a nice Hasher class for this: return Hasher().computeHash(m_host, m_protocol, m_port);
Created attachment 333100 [details] the patch
Attachment 333100 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:198: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/ThreadLocalCache.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 2 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 333104 [details] the patch
Attachment 333104 [details] did not pass style-queue: ERROR: Source/WebCore/dom/Document.cpp:198: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/ThreadLocalCache.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 2 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 333104 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=333104&action=review > Source/WebCore/ChangeLog:5 > +2018-02-05 Filip Pizlo <fpizlo@apple.com> > + > + Global objects should be able to use TLCs to allocate from different blocks from each other > + https://bugs.webkit.org/show_bug.cgi?id=182227 > + Please fix up this ChangeLog file as it contains three added entries. > Source/WebCore/dom/Document.cpp:5553 > - securityOrigin().enforceFilePathSeparation(); > + securityOrigin().setEnforcesFilePathSeparation(true); I know that you changed this to be a setter so as to expose a getter with a similar name to query whether file path separation is enabled in Document::threadLocalCache(). I prefer that we keep the old style one-way only setter because we do not want to advertise to WebKit developers the ability to change the enforcement of file path separation after it is set as this is error prone. One way to allow Document::threadLocalCache() to make the correct policy decision is to add a member function to SecurityOrigin, maybe canAccessSharedThreadLocalCache or needsIsolatedThreadLocalCache, that returns true for a unique origin or a local origin with file path separation and false otherwise (it would be good to explicitly break out the m_universalAccess - no same-origin restrictions - case to return false as a means to document that this was intentional). Then Document::threadLocalCache() can query canAccessSharedThreadLocalCache(). Additional remarks: The general philosophy around SecurityOrigin, the file path separation enforcement, and other security policies in the engine is that we do not want restrictions to be removed once added. > Source/WebCore/dom/Document.cpp:7749 > + if (origin.isUnique() || (origin.isLocal() && origin.enforcesFilePathSeparation())) See my remark above about adding a new member function to SecurityOrigin to query for this policy. > Source/WebCore/page/SecurityOrigin.h:169 > + void setEnforcesFilePathSeparation(bool); > + bool enforcesFilePathSeparation() const { return m_enforcesFilePathSeparation; } I would prefer that we do not make this change. See my remarks in Document.cpp for more details. > Source/WebCore/page/ThreadLocalCacheOriginKey.h:96 > +class ThreadLocalCacheOriginKey { > +public: > + ThreadLocalCacheOriginKey() > + { > + } > + > + ThreadLocalCacheOriginKey(String host, String protocol, std::optional<uint16_t> port) > + : m_host(host) > + , m_protocol(protocol) > + , m_port(port) > + { > + } > + > + ThreadLocalCacheOriginKey(WTF::HashTableDeletedValueType) > + : m_host(WTF::HashTableDeletedValue) > + { > + } > + > + bool operator==(const ThreadLocalCacheOriginKey& other) const > + { > + return m_host == other.m_host > + && m_protocol == other.m_protocol > + && m_port == other.m_port; > + } > + > + bool operator!=(const ThreadLocalCacheOriginKey& other) const > + { > + return !(*this == other); > + } > + > + explicit operator bool() const > + { > + return *this != ThreadLocalCacheOriginKey(); > + } > + > + bool isHashTableDeletedValue() const { return m_host.isHashTableDeletedValue(); } > + > + unsigned hash() const > + { > + return m_host.hash() + m_protocol.hash() + (m_port ? *m_port : 0); > + } > + > +private: > + String m_host; > + String m_protocol; > + std::optional<uint16_t> m_port; > +}; > + > +struct ThreadLocalCacheOriginKeyHash { > + static unsigned hash(const ThreadLocalCacheOriginKey& key) > + { > + return key.hash(); > + } > + > + static bool equal(const ThreadLocalCacheOriginKey& a, const ThreadLocalCacheOriginKey& b) > + { > + return a == b; > + } > + > + static const bool safeToCompareToEmptyOrDeleted = false; // This is a safe choice, possibly more conservative than necessary. > +}; > + These classes duplicate the functionality of SecurityOriginHash. We should use SecurityOriginHash: <https://trac.webkit.org/browser/trunk/Source/WebCore/page/SecurityOriginHash.h>.
(In reply to Daniel Bates from comment #30) > Comment on attachment 333104 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333104&action=review > > > Source/WebCore/ChangeLog:5 > > +2018-02-05 Filip Pizlo <fpizlo@apple.com> > > + > > + Global objects should be able to use TLCs to allocate from different blocks from each other > > + https://bugs.webkit.org/show_bug.cgi?id=182227 > > + > > Please fix up this ChangeLog file as it contains three added entries. Will do. > > > Source/WebCore/dom/Document.cpp:5553 > > - securityOrigin().enforceFilePathSeparation(); > > + securityOrigin().setEnforcesFilePathSeparation(true); > > I know that you changed this to be a setter so as to expose a getter with a > similar name to query whether file path separation is enabled in > Document::threadLocalCache(). I prefer that we keep the old style one-way > only setter because we do not want to advertise to WebKit developers the > ability to change the enforcement of file path separation after it is set as > this is error prone. In its current form, the patch requires that the argument to setEnforcesBlah to be true due to an assertion. Sounds like you're saying it would be better if it did not take a bool at all. I think that removing the bool arg to setEnforcesFilePathSeparation() addresses this specific concern. > One way to allow Document::threadLocalCache() to make > the correct policy decision is to add a member function to SecurityOrigin, > maybe canAccessSharedThreadLocalCache or needsIsolatedThreadLocalCache, that > returns true for a unique origin or a local origin with file path separation > and false otherwise (it would be good to explicitly break out the > m_universalAccess - no same-origin restrictions - case to return false as a > means to document that this was intentional). Then > Document::threadLocalCache() can query canAccessSharedThreadLocalCache(). I'll leave this refinement for another patch. I don't see the harm in having a getter for this property. > > Additional remarks: > > The general philosophy around SecurityOrigin, the file path separation > enforcement, and other security policies in the engine is that we do not > want restrictions to be removed once added. This patch doesn't change that philosophy in any way. The fact that the set method takes a bool is symbolic since you're not allowed to pass anything but true. > > > Source/WebCore/dom/Document.cpp:7749 > > + if (origin.isUnique() || (origin.isLocal() && origin.enforcesFilePathSeparation())) > > See my remark above about adding a new member function to SecurityOrigin to > query for this policy. > > > Source/WebCore/page/SecurityOrigin.h:169 > > + void setEnforcesFilePathSeparation(bool); > > + bool enforcesFilePathSeparation() const { return m_enforcesFilePathSeparation; } > > I would prefer that we do not make this change. See my remarks in > Document.cpp for more details. > > > Source/WebCore/page/ThreadLocalCacheOriginKey.h:96 > > +class ThreadLocalCacheOriginKey { > > +public: > > + ThreadLocalCacheOriginKey() > > + { > > + } > > + > > + ThreadLocalCacheOriginKey(String host, String protocol, std::optional<uint16_t> port) > > + : m_host(host) > > + , m_protocol(protocol) > > + , m_port(port) > > + { > > + } > > + > > + ThreadLocalCacheOriginKey(WTF::HashTableDeletedValueType) > > + : m_host(WTF::HashTableDeletedValue) > > + { > > + } > > + > > + bool operator==(const ThreadLocalCacheOriginKey& other) const > > + { > > + return m_host == other.m_host > > + && m_protocol == other.m_protocol > > + && m_port == other.m_port; > > + } > > + > > + bool operator!=(const ThreadLocalCacheOriginKey& other) const > > + { > > + return !(*this == other); > > + } > > + > > + explicit operator bool() const > > + { > > + return *this != ThreadLocalCacheOriginKey(); > > + } > > + > > + bool isHashTableDeletedValue() const { return m_host.isHashTableDeletedValue(); } > > + > > + unsigned hash() const > > + { > > + return m_host.hash() + m_protocol.hash() + (m_port ? *m_port : 0); > > + } > > + > > +private: > > + String m_host; > > + String m_protocol; > > + std::optional<uint16_t> m_port; > > +}; > > + > > +struct ThreadLocalCacheOriginKeyHash { > > + static unsigned hash(const ThreadLocalCacheOriginKey& key) > > + { > > + return key.hash(); > > + } > > + > > + static bool equal(const ThreadLocalCacheOriginKey& a, const ThreadLocalCacheOriginKey& b) > > + { > > + return a == b; > > + } > > + > > + static const bool safeToCompareToEmptyOrDeleted = false; // This is a safe choice, possibly more conservative than necessary. > > +}; > > + > > These classes duplicate the functionality of SecurityOriginHash. We should > use SecurityOriginHash: > <https://trac.webkit.org/browser/trunk/Source/WebCore/page/ > SecurityOriginHash.h>. That's only true if you believe that keeping a RefPtr to a SecurityOrigin is only safe - that is, that the SecurityOrigin won't keep a lot of other stuff alive. Is that true?
(In reply to Filip Pizlo from comment #31) > (In reply to Daniel Bates from comment #30) > > Comment on attachment 333104 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=333104&action=review > > > > > Source/WebCore/ChangeLog:5 > > > +2018-02-05 Filip Pizlo <fpizlo@apple.com> > > > + > > > + Global objects should be able to use TLCs to allocate from different blocks from each other > > > + https://bugs.webkit.org/show_bug.cgi?id=182227 > > > + > > > > Please fix up this ChangeLog file as it contains three added entries. > > Will do. > > > > > > Source/WebCore/dom/Document.cpp:5553 > > > - securityOrigin().enforceFilePathSeparation(); > > > + securityOrigin().setEnforcesFilePathSeparation(true); > > > > I know that you changed this to be a setter so as to expose a getter with a > > similar name to query whether file path separation is enabled in > > Document::threadLocalCache(). I prefer that we keep the old style one-way > > only setter because we do not want to advertise to WebKit developers the > > ability to change the enforcement of file path separation after it is set as > > this is error prone. > > In its current form, the patch requires that the argument to setEnforcesBlah > to be true due to an assertion. Sounds like you're saying it would be > better if it did not take a bool at all. I think that removing the bool arg > to setEnforcesFilePathSeparation() addresses this specific concern. > > OK. > > One way to allow Document::threadLocalCache() to make > > the correct policy decision is to add a member function to SecurityOrigin, > > maybe canAccessSharedThreadLocalCache or needsIsolatedThreadLocalCache, that > > returns true for a unique origin or a local origin with file path separation > > and false otherwise (it would be good to explicitly break out the > > m_universalAccess - no same-origin restrictions - case to return false as a > > means to document that this was intentional). Then > > Document::threadLocalCache() can query canAccessSharedThreadLocalCache(). > > I'll leave this refinement for another patch. I don't see the harm in > having a getter for this property. > OK. > > > > Additional remarks: > > > > The general philosophy around SecurityOrigin, the file path separation > > enforcement, and other security policies in the engine is that we do not > > want restrictions to be removed once added. > > This patch doesn't change that philosophy in any way. The fact that the set > method takes a bool is symbolic since you're not allowed to pass anything > but true. > > > > > > Source/WebCore/dom/Document.cpp:7749 > > > + if (origin.isUnique() || (origin.isLocal() && origin.enforcesFilePathSeparation())) > > > > See my remark above about adding a new member function to SecurityOrigin to > > query for this policy. > > > > > Source/WebCore/page/SecurityOrigin.h:169 > > > + void setEnforcesFilePathSeparation(bool); > > > + bool enforcesFilePathSeparation() const { return m_enforcesFilePathSeparation; } > > > > I would prefer that we do not make this change. See my remarks in > > Document.cpp for more details. > > > > > Source/WebCore/page/ThreadLocalCacheOriginKey.h:96 > > > +class ThreadLocalCacheOriginKey { > > > +public: > > > + ThreadLocalCacheOriginKey() > > > + { > > > + } > > > + > > > + ThreadLocalCacheOriginKey(String host, String protocol, std::optional<uint16_t> port) > > > + : m_host(host) > > > + , m_protocol(protocol) > > > + , m_port(port) > > > + { > > > + } > > > + > > > + ThreadLocalCacheOriginKey(WTF::HashTableDeletedValueType) > > > + : m_host(WTF::HashTableDeletedValue) > > > + { > > > + } > > > + > > > + bool operator==(const ThreadLocalCacheOriginKey& other) const > > > + { > > > + return m_host == other.m_host > > > + && m_protocol == other.m_protocol > > > + && m_port == other.m_port; > > > + } > > > + > > > + bool operator!=(const ThreadLocalCacheOriginKey& other) const > > > + { > > > + return !(*this == other); > > > + } > > > + > > > + explicit operator bool() const > > > + { > > > + return *this != ThreadLocalCacheOriginKey(); > > > + } > > > + > > > + bool isHashTableDeletedValue() const { return m_host.isHashTableDeletedValue(); } > > > + > > > + unsigned hash() const > > > + { > > > + return m_host.hash() + m_protocol.hash() + (m_port ? *m_port : 0); > > > + } > > > + > > > +private: > > > + String m_host; > > > + String m_protocol; > > > + std::optional<uint16_t> m_port; > > > +}; > > > + > > > +struct ThreadLocalCacheOriginKeyHash { > > > + static unsigned hash(const ThreadLocalCacheOriginKey& key) > > > + { > > > + return key.hash(); > > > + } > > > + > > > + static bool equal(const ThreadLocalCacheOriginKey& a, const ThreadLocalCacheOriginKey& b) > > > + { > > > + return a == b; > > > + } > > > + > > > + static const bool safeToCompareToEmptyOrDeleted = false; // This is a safe choice, possibly more conservative than necessary. > > > +}; > > > + > > > > These classes duplicate the functionality of SecurityOriginHash. We should > > use SecurityOriginHash: > > <https://trac.webkit.org/browser/trunk/Source/WebCore/page/ > > SecurityOriginHash.h>. > > That's only true if you believe that keeping a RefPtr to a SecurityOrigin is > only safe - that is, that the SecurityOrigin won't keep a lot of other stuff > alive. > > Is that true? Holding a SecurityOrigin should not keep other things alive. I do not want to hold up this patch. If there is a way to share more code with SecurityOriginHash that would be great. At the very least, can we please standardize on protocol, host, port as the ordering for creating the keys.
(In reply to Daniel Bates from comment #32) > (In reply to Filip Pizlo from comment #31) > > (In reply to Daniel Bates from comment #30) > > > Comment on attachment 333104 [details] > > > the patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=333104&action=review > > > > > > > Source/WebCore/ChangeLog:5 > > > > +2018-02-05 Filip Pizlo <fpizlo@apple.com> > > > > + > > > > + Global objects should be able to use TLCs to allocate from different blocks from each other > > > > + https://bugs.webkit.org/show_bug.cgi?id=182227 > > > > + > > > > > > Please fix up this ChangeLog file as it contains three added entries. > > > > Will do. > > > > > > > > > Source/WebCore/dom/Document.cpp:5553 > > > > - securityOrigin().enforceFilePathSeparation(); > > > > + securityOrigin().setEnforcesFilePathSeparation(true); > > > > > > I know that you changed this to be a setter so as to expose a getter with a > > > similar name to query whether file path separation is enabled in > > > Document::threadLocalCache(). I prefer that we keep the old style one-way > > > only setter because we do not want to advertise to WebKit developers the > > > ability to change the enforcement of file path separation after it is set as > > > this is error prone. > > > > In its current form, the patch requires that the argument to setEnforcesBlah > > to be true due to an assertion. Sounds like you're saying it would be > > better if it did not take a bool at all. I think that removing the bool arg > > to setEnforcesFilePathSeparation() addresses this specific concern. > > > > > > OK. > > > > One way to allow Document::threadLocalCache() to make > > > the correct policy decision is to add a member function to SecurityOrigin, > > > maybe canAccessSharedThreadLocalCache or needsIsolatedThreadLocalCache, that > > > returns true for a unique origin or a local origin with file path separation > > > and false otherwise (it would be good to explicitly break out the > > > m_universalAccess - no same-origin restrictions - case to return false as a > > > means to document that this was intentional). Then > > > Document::threadLocalCache() can query canAccessSharedThreadLocalCache(). > > > > I'll leave this refinement for another patch. I don't see the harm in > > having a getter for this property. > > > > OK. > > > > > > > Additional remarks: > > > > > > The general philosophy around SecurityOrigin, the file path separation > > > enforcement, and other security policies in the engine is that we do not > > > want restrictions to be removed once added. > > > > This patch doesn't change that philosophy in any way. The fact that the set > > method takes a bool is symbolic since you're not allowed to pass anything > > but true. > > > > > > > > > Source/WebCore/dom/Document.cpp:7749 > > > > + if (origin.isUnique() || (origin.isLocal() && origin.enforcesFilePathSeparation())) > > > > > > See my remark above about adding a new member function to SecurityOrigin to > > > query for this policy. > > > > > > > Source/WebCore/page/SecurityOrigin.h:169 > > > > + void setEnforcesFilePathSeparation(bool); > > > > + bool enforcesFilePathSeparation() const { return m_enforcesFilePathSeparation; } > > > > > > I would prefer that we do not make this change. See my remarks in > > > Document.cpp for more details. > > > > > > > Source/WebCore/page/ThreadLocalCacheOriginKey.h:96 > > > > +class ThreadLocalCacheOriginKey { > > > > +public: > > > > + ThreadLocalCacheOriginKey() > > > > + { > > > > + } > > > > + > > > > + ThreadLocalCacheOriginKey(String host, String protocol, std::optional<uint16_t> port) > > > > + : m_host(host) > > > > + , m_protocol(protocol) > > > > + , m_port(port) > > > > + { > > > > + } > > > > + > > > > + ThreadLocalCacheOriginKey(WTF::HashTableDeletedValueType) > > > > + : m_host(WTF::HashTableDeletedValue) > > > > + { > > > > + } > > > > + > > > > + bool operator==(const ThreadLocalCacheOriginKey& other) const > > > > + { > > > > + return m_host == other.m_host > > > > + && m_protocol == other.m_protocol > > > > + && m_port == other.m_port; > > > > + } > > > > + > > > > + bool operator!=(const ThreadLocalCacheOriginKey& other) const > > > > + { > > > > + return !(*this == other); > > > > + } > > > > + > > > > + explicit operator bool() const > > > > + { > > > > + return *this != ThreadLocalCacheOriginKey(); > > > > + } > > > > + > > > > + bool isHashTableDeletedValue() const { return m_host.isHashTableDeletedValue(); } > > > > + > > > > + unsigned hash() const > > > > + { > > > > + return m_host.hash() + m_protocol.hash() + (m_port ? *m_port : 0); > > > > + } > > > > + > > > > +private: > > > > + String m_host; > > > > + String m_protocol; > > > > + std::optional<uint16_t> m_port; > > > > +}; > > > > + > > > > +struct ThreadLocalCacheOriginKeyHash { > > > > + static unsigned hash(const ThreadLocalCacheOriginKey& key) > > > > + { > > > > + return key.hash(); > > > > + } > > > > + > > > > + static bool equal(const ThreadLocalCacheOriginKey& a, const ThreadLocalCacheOriginKey& b) > > > > + { > > > > + return a == b; > > > > + } > > > > + > > > > + static const bool safeToCompareToEmptyOrDeleted = false; // This is a safe choice, possibly more conservative than necessary. > > > > +}; > > > > + > > > > > > These classes duplicate the functionality of SecurityOriginHash. We should > > > use SecurityOriginHash: > > > <https://trac.webkit.org/browser/trunk/Source/WebCore/page/ > > > SecurityOriginHash.h>. > > > > That's only true if you believe that keeping a RefPtr to a SecurityOrigin is > > only safe - that is, that the SecurityOrigin won't keep a lot of other stuff > > alive. > > > > Is that true? > > Holding a SecurityOrigin should not keep other things alive. I do not want > to hold up this patch. If there is a way to share more code with > SecurityOriginHash that would be great. At the very least, can we please > standardize on protocol, host, port as the ordering for creating the keys. OK! I'm going to make this change, since it makes the patch smaller. I'm making a patch that makes the setter not take a bool, and switch to RefPtr<SecurityOrigin>. Will have it up shortly.
Comment on attachment 333104 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=333104&action=review Thank you Filip for iterating on this patch! I am optimistic that we can further refine this code. I do not feel it is necessary to hold up this patch for such refinements. We can do them in subsequent bugs/patches. This patch does not seem to incorporate some or all of the feedback from Chris Dumez in comment 25 and I did not see a reply from you against such feedback. Please address these remarks or explain why you disagree with them. r=me >>> Source/WebCore/dom/Document.cpp:5553 >>> + securityOrigin().setEnforcesFilePathSeparation(true); >> >> I know that you changed this to be a setter so as to expose a getter with a similar name to query whether file path separation is enabled in Document::threadLocalCache(). I prefer that we keep the old style one-way only setter because we do not want to advertise to WebKit developers the ability to change the enforcement of file path separation after it is set as this is error prone. One way to allow Document::threadLocalCache() to make the correct policy decision is to add a member function to SecurityOrigin, maybe canAccessSharedThreadLocalCache or needsIsolatedThreadLocalCache, that returns true for a unique origin or a local origin with file path separation and false otherwise (it would be good to explicitly break out the m_universalAccess - no same-origin restrictions - case to return false as a means to document that this was intentional). Then Document::threadLocalCache() can query canAccessSharedThreadLocalCache(). >> >> Additional remarks: >> >> The general philosophy around SecurityOrigin, the file path separation enforcement, and other security policies in the engine is that we do not want restrictions to be removed once added. > > In its current form, the patch requires that the argument to setEnforcesBlah to be true due to an assertion. Sounds like you're saying it would be better if it did not take a bool at all. I think that removing the bool arg to setEnforcesFilePathSeparation() addresses this specific concern. That works for me.
(In reply to Chris Dumez from comment #25) > Comment on attachment 332810 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332810&action=review > > I only looked at the WebCore bits and it look good overall (a few comments > though). > > > Source/WebCore/ChangeLog:1 > > +2018-01-30 Filip Pizlo <fpizlo@apple.com> > > Double change log. Fixed. > > > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:59 > > +JSDOMGlobalObject::JSDOMGlobalObject(VM& vm, Structure* structure, Ref<DOMWrapperWorld>&& world, const GlobalObjectMethodTable* globalObjectMethodTable, RefPtr<JSC::ThreadLocalCache> threadLocalCache) > > We usually use && for parameters like these. Fixed. > > > Source/WebCore/bindings/js/JSDOMWindowBase.cpp:82 > > + : JSDOMGlobalObject(vm, structure, proxy->world(), &s_globalObjectMethodTable, window->document()->securityOrigin().threadLocalCache()) > > The next line makes me worry about window potentially being null? I'm worried that the next line is being too paranoid. But I'll play along. The obvious thing to do is "window ? window->blah.threadLocalCache() : nullptr", so I'll do that. > > > Source/WebCore/page/OriginThreadLocalCache.cpp:33 > > +typedef HashMap<ThreadLocalCacheOriginKey, OriginThreadLocalCache*> ThreadLocalCacheMap; > > I believe we started transitioning to "using" statements in WebCore: > using ThreadLocalCacheMap = HashMap<ThreadLocalCacheOriginKey, > OriginThreadLocalCache*>; Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.cpp:41 > > +RefPtr<OriginThreadLocalCache> OriginThreadLocalCache::create(const ThreadLocalCacheOriginKey& key) > > Ref<OriginThreadLocalCache> Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.cpp:45 > > + return iter->value; > > return *iter->value; Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.cpp:47 > > + return adoptRef(new OriginThreadLocalCache(key)); > > return adoptRef(*new OriginThreadLocalCache(key)); Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.cpp:52 > > + dataLog(RawPointer(this), ": destroying origin TLC.\n"); > > should probably be removed. Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.cpp:62 > > + RELEASE_ASSERT(result); > > result is a AddResult. Did you mean RELEASE_ASSERT(result.isNewEntry) ? You don't need to say .isNewEntry if you're using AddResult in a boolean context: template<typename IteratorType> struct HashTableAddResult { ... explicit operator bool() const { return isNewEntry; } > > > Source/WebCore/page/OriginThreadLocalCache.cpp:63 > > + dataLog(RawPointer(this), ": creating origin TLC.\n"); > > should probably be removed. Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.h:30 > > +#include <wtf/HashMap.h> > > Move to cpp? Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.h:31 > > +#include <wtf/NeverDestroyed.h> > > Move to cpp? Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.h:35 > > +class OriginThreadLocalCache : public JSC::ThreadLocalCache { > > Should this be marked as final? Good idea. Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.h:37 > > + static RefPtr<OriginThreadLocalCache> create(const ThreadLocalCacheOriginKey&); > > This can never return nullptr, right? If so, it should return a Ref<>, not a > RefPtr<>. Fixed. > > > Source/WebCore/page/OriginThreadLocalCache.h:42 > > + OriginThreadLocalCache(const ThreadLocalCacheOriginKey&); > > explicit? Fixed. > > > Source/WebCore/page/SecurityOrigin.cpp:602 > > +RefPtr<JSC::ThreadLocalCache> SecurityOrigin::threadLocalCache() > > We're not transferring ownership to the caller, I believe we would usually > return a JSC::ThreadLocalCache& in such cases. I see. Fixed. > > > Source/WebCore/page/ThreadLocalCacheOriginKey.h:41 > > + ThreadLocalCacheOriginKey(String host, String protocol, std::optional<uint16_t> port) > > we usually pass strings as "const String&". Fixed by virtue of removing ThreadLocalCacheOriginKey.h per Dan's comment - this was just duplicating SecurityOriginHash. > > > Source/WebCore/page/ThreadLocalCacheOriginKey.h:74 > > + return m_host.hash() + m_protocol.hash() + (m_port ? *m_port : 0); > > I guess this is fine but I think we have a nice Hasher class for this: > return Hasher().computeHash(m_host, m_protocol, m_port); Fixed by removal.
(In reply to Daniel Bates from comment #34) > Comment on attachment 333104 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333104&action=review > > Thank you Filip for iterating on this patch! I am optimistic that we can > further refine this code. I do not feel it is necessary to hold up this > patch for such refinements. We can do them in subsequent bugs/patches. > > This patch does not seem to incorporate some or all of the feedback from > Chris Dumez in comment 25 and I did not see a reply from you against such > feedback. Please address these remarks or explain why you disagree with them. Thanks for reminding me about Chris's comment. I had missed it. I addressed his feedback. > > r=me > > >>> Source/WebCore/dom/Document.cpp:5553 > >>> + securityOrigin().setEnforcesFilePathSeparation(true); > >> > >> I know that you changed this to be a setter so as to expose a getter with a similar name to query whether file path separation is enabled in Document::threadLocalCache(). I prefer that we keep the old style one-way only setter because we do not want to advertise to WebKit developers the ability to change the enforcement of file path separation after it is set as this is error prone. One way to allow Document::threadLocalCache() to make the correct policy decision is to add a member function to SecurityOrigin, maybe canAccessSharedThreadLocalCache or needsIsolatedThreadLocalCache, that returns true for a unique origin or a local origin with file path separation and false otherwise (it would be good to explicitly break out the m_universalAccess - no same-origin restrictions - case to return false as a means to document that this was intentional). Then Document::threadLocalCache() can query canAccessSharedThreadLocalCache(). > >> > >> Additional remarks: > >> > >> The general philosophy around SecurityOrigin, the file path separation enforcement, and other security policies in the engine is that we do not want restrictions to be removed once added. > > > > In its current form, the patch requires that the argument to setEnforcesBlah to be true due to an assertion. Sounds like you're saying it would be better if it did not take a bool at all. I think that removing the bool arg to setEnforcesFilePathSeparation() addresses this specific concern. > > That works for me.
Created attachment 333138 [details] patch for landing Addressed all the feedback.
Created attachment 333139 [details] patch for landing Improved changelog.
Attachment 333139 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/ThreadLocalCache.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Landed in https://trac.webkit.org/changeset/228149/webkit
Comment on attachment 333139 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=333139&action=review > Source/WebCore/page/OriginThreadLocalCache.cpp:36 > +typedef HashMap<RefPtr<SecurityOrigin>, OriginThreadLocalCache*> ThreadLocalCacheMap; So the key is a pointer and I see to remember that we can construct different SecurityOrigin objects for the same underlying origin, so how does this work? The previous iteration of the patch that I reviewed was actually dealing with this and using something else as key. Doesn't this create a new TLC for every new document, even if in the same origin?
Comment on attachment 333139 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=333139&action=review >> Source/WebCore/page/OriginThreadLocalCache.cpp:36 >> +typedef HashMap<RefPtr<SecurityOrigin>, OriginThreadLocalCache*> ThreadLocalCacheMap; > > So the key is a pointer and I see to remember that we can construct different SecurityOrigin objects for the same underlying origin, so how does this work? The previous iteration of the patch that I reviewed was actually dealing with this and using something else as key. > > Doesn't this create a new TLC for every new document, even if in the same origin? Oh, I see now Daniel's comment about SecurityOriginHash. I did not know we hashed RefPtr<SecurityOrigin> differently than regular pointers.