[webkit-reviews] review granted: [Bug 182227] Global objects should be able to use TLCs to allocate from different blocks from each other : [Attachment 333104] the patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 182227: 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

Attachment 333104: the patch

https://bugs.webkit.org/attachment.cgi?id=333104&action=review




--- Comment #34 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 333104
  --> https://bugs.webkit.org/attachment.cgi?id=333104
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.