[webkit-reviews] review denied: [Bug 182664] Resource Load Statistics: Classify resources as prevalent based on redirects to other prevalent resources : [Attachment 333539] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied	review:
Bug 182664: Resource Load Statistics: Classify resources as prevalent based on
redirects to other prevalent resources
https://bugs.webkit.org/show_bug.cgi?id=182664

Attachment 333539: Patch

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




--- Comment #6 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 333539
  --> https://bugs.webkit.org/attachment.cgi?id=333539
Patch

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

Now that I understand better, this really was introducing a nasty bug. I'm
changing to r- for you to fix. :-)

>>> Source/WebCore/loader/ResourceLoadObserver.cpp:167
>>> +	 auto& targetStatistics =
ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);
>> 
>> I don't see why this line had to be moved. It makes me nervous, because we
have had many threading/memory corruption bugs caused by invalidated iterators,
and I'd prefer to limit the scope of these references to the smaller area
possible. It doesn't seem like this is referenced again in your new code below,
so I don't think you should make this change.
>> 
>> Or did you do this to make sure there is an existing entry for the top frame
in the redirect code below? If so, I propose you do so at the point you need
it, and add the same comment you do elsewhere ("For consistency, make sure we
also have a statistics entry for the top frame domain."). I would also give it
the name "topFrameStatistics" in the redirect sections below.
> 
> I see. However, I do use targetStatistics in two more places now, which is
why I moved it. Please see new lines 182 and 185 where I now add
topFrameUniqueRedirectsFrom and subresourceUniqueRedirectsFrom respectively.
> 
> Previously we only added statistics to the redirecting domain but now we need
to keep track of both directions for the purpose of traversing the redirect
graph backwards.
> 
> Please advice.

Ah! I missed that.

This is dangerous, because both calls to
"ensureResourceStatisticsForPrimaryDomain" potentially insert content into the
HashSet, invalidating the reference you obtained earlier.

I think in the redirect case you need to change to retrieving a copy of the
content, then setting it back in the HashSet to avoid creating another set of
difficult-to-debug crashes.

So:

1. Leave the original targetStatistics code on line 172 alone. That one is
fine.
2. In the redirect section, do one of these two things:

(a) Assign the return value of the 'ensureResourceStatisticsForPrimaryDomain"
to a temporary value (a copy), and then set the value back in the
m_resourceStatisticsMap,
-or-
(b) Update the 'redirectingOriginStatistics" as you have done, then do a call
to "ensureResourceStatistics..." to a local reference and then update the
'targetStatistics' value.

If you don't the 'targetStatistics' object will be potentially deallocated
memory and your use of it will bring about the end of the world. Or at least,
will cause some nasty security bugs.

> Source/WebCore/loader/ResourceLoadObserver.cpp:181
> +	       isNewRedirectToEntry =
redirectingOriginStatistics.topFrameUniqueRedirectsTo.add(targetPrimaryDomain).
isNewEntry;

So I suggest that immediately after line 181 you do:

auto& targetStatistics =
ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

> Source/WebCore/loader/ResourceLoadObserver.cpp:184
> +	       isNewRedirectToEntry =
redirectingOriginStatistics.subresourceUniqueRedirectsTo.add(targetPrimaryDomai
n).isNewEntry;

Ditto, right here after line 184:

auto& targetStatistics =
ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);