[webkit-reviews] review denied: [Bug 172519] [WK2] Address thread safety issues with ResourceLoadStatistics : [Attachment 311246] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 172519: [WK2] Address thread safety issues with ResourceLoadStatistics
https://bugs.webkit.org/show_bug.cgi?id=172519

Attachment 311246: Patch

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




--- Comment #35 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 311246
  --> https://bugs.webkit.org/attachment.cgi?id=311246
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:69
> +    m_queue = WTFMove(queue);

Can we assert that  ASSERT(!m_queue); before doing the assignment?

> Source/WebCore/loader/ResourceLoadObserver.cpp:74
> +    if (!m_store)

Can you explain why we assert that we have a queue but we have an if check to
see if we have a store? It seems we always set the store first, then the queue.
So I would think an assertion would suffice.

> Source/WebCore/loader/ResourceLoadObserver.cpp:85
> +    if (!m_store)

ditto

> Source/WebCore/loader/ResourceLoadObserver.cpp:151
> +	   auto targetStatistics =
m_store->ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

Are we making a copy of the targetStatistics here? It is not 100% clear to me
due to the use of auto, but I guess we are? If we are not, then it is not
thread safe.

> Source/WebCore/loader/ResourceLoadObserver.cpp:168
> +	       auto& redirectingOriginResourceStatistics =
m_store->ensureResourceStatisticsForPrimaryDomain(sourcePrimaryDomain);

Nothing prevents the main thread from modifying the store's hash map after
you've retrieved a reference to one of the hash table's values. This means that
your reference (targetStatistics) may get invalidated at any point later in
this function.

> Source/WebCore/loader/ResourceLoadObserver.cpp:190
> +		   auto& sourceOriginResourceStatistics =
m_store->ensureResourceStatisticsForPrimaryDomain(sourcePrimaryDomain);

ditto here.

> Source/WebCore/loader/ResourceLoadObserver.cpp:205
> +		   m_store->fireDataModificationHandler();

Could we add an assertion in fireDataModificationHandler() to make sure it is
always called on the main thread? I don't see one.

> Source/WebCore/loader/ResourceLoadObserver.cpp:242
> +	   auto& targetStatistics =
m_store->ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

ditto here about your reference potentially getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:317
> +	   auto& targetStatistics =
m_store->ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

Ditto about your reference getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:361
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomainString);

ditto about your reference getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:384
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomainString);

Ditto about your reference getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:389
> +	       m_store->fireShouldPartitionCookiesHandler({ primaryDomainString
}, { }, false);

Could we add an assert in fireShouldPartitionCookiesHandler() to make sure it
is always called on the main thread? I do not see one.

> Source/WebCore/loader/ResourceLoadObserver.cpp:403
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomainString);

Ditto about your reference getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:416
>      auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(url));

See here an example of the main thread modifying the store's hash table while
the statistic's thread is potentially holding a reference to one of the hash
table's values. In case of re-hashing for e.g., that reference will be
invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:430
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomainString);

ditto here about the reference potentially getting invalidated before the next
line of code.

> Source/WebCore/loader/ResourceLoadObserver.cpp:455
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomainString);

ditto about your reference getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:469
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomainString);

ditto about your reference getting invalidated.

So we the current design (dispatch for setting, lock for retrieving), if you
do:
setGrandfathered(url, true);
v = isGrandfathered(url); // v may still be false I believe

Is this an issue?

> Source/WebCore/loader/ResourceLoadObserver.cpp:495
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primarySubFrameDomainString);

ditto about your reference potentially getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:510
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primarySubresourceDomainStrin
g);

ditto about your reference potentially getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:525
> +	   auto& statistics =
m_store->ensureResourceStatisticsForPrimaryDomain(primarySubresourceDomainStrin
g);

ditto about your reference potentially getting invalidated.

> Source/WebCore/loader/ResourceLoadObserver.cpp:599
> +    if (!m_store)

Can m_store ever be null?

> Source/WebCore/loader/ResourceLoadObserver.cpp:602
> +    auto locker = holdLock(m_store->statisticsLock());

So here you lock and then store::statisticsForOrigin() will also lock. Is this
OK? Do we really need this lock?

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:67
> +    auto locker = holdLock(m_statisticsLock);

Even though you lock here before accessing the hashMap, you return an iterator
to that hash map and then we unlock. This does not look safe given that the
iterator may be invalidated at any point after you drop the lock.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:90
>      encoder->encodeDouble("endOfGrandfatheringTimestamp",
m_endOfGrandfatheringTimestamp);

Is it really OK to use m_endOfGrandfatheringTimestamp before locking?

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:141
>      fireShouldPartitionCookiesHandler({ },
prevalentResourceDomainsWithoutUserInteraction, true);

fireShouldPartitionCookiesHandler is called from a background thread here but
you explicitly called it on the main thread earlier in this patch. Is this OK?

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:151
>      fireShouldPartitionCookiesHandler({ }, { }, true);

fireShouldPartitionCookiesHandler is called from a background thread here but
you explicitly called it on the main thread earlier in this patch. Is this OK?

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:190
>  void ResourceLoadStatisticsStore::mergeStatistics(const
Vector<ResourceLoadStatistics>& statistics)

FYI, I see that ResourceLoadStatisticsStore::setNotificationCallback() below is
using std::function below. It's be good to make sure we use WTF::Function
everywhere, especially considering we're doing multithreading. std::function
does implicit copies on the captured variables so calling isolatedCopy() when
capturing is not enough. We have been caught before and this is why we have
WTF::Function now.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:303
>  void
ResourceLoadStatisticsStore::processStatistics(std::function<void(ResourceLoadS
tatistics&)>&& processFunction)

Should probably use WTF::Function.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:373
>      if (m_dataRecordsRemovalPending)

Can we add an assertion here to make sure this is always called on a non-main
thread since m_dataRecordsRemovalPending is not protected by locks and is
always updated on a background thread?

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:-361
> -

Can we add assertions above in dataRecordsBeingRemoved() /
dataRecordsWereRemoved() to make sure they're always called on a background
thread?