[webkit-reviews] review denied: [Bug 180602] Crash in WTF::Atomic<unsigned char>::compareExchangeWeak + 36 : [Attachment 329144] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied John Wilander
<wilander at apple.com>'s request for review:
Bug 180602: Crash in WTF::Atomic<unsigned char>::compareExchangeWeak + 36
https://bugs.webkit.org/show_bug.cgi?id=180602

Attachment 329144: Patch

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




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

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

This looks right, but you are copying a bunch of stuff we can just move. Please
switch to move so we don't waste energy copying these values.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:55
> +    PrevalentResourceTelemetry isolatedCopy() const

Do we need a special cross-site copy for these atomic types?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:221
> +    RunLoop::main().dispatch([totalPrevalentResources =
crossThreadCopy(totalPrevalentResources),
totalPrevalentResourcesWithUserInteraction =
crossThreadCopy(totalPrevalentResourcesWithUserInteraction),
top3SubframeUnderTopFrameOrigins =
crossThreadCopy(top3SubframeUnderTopFrameOrigins)] {

I just ran this by Chris, and confirmed we should be doing WTFMove() for
vectors of atomic types (e.g., prevalentResourcesDaysSinceUserInteraction).

Because we never use these values agin, I think you can just WTFMove() all of
them, without needing the copy.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:262
> +    RunLoop::main().dispatch([sortedPrevalentResources =
crossThreadCopy(sortedPrevalentResources),
sortedPrevalentResourcesWithoutUserInteraction =
crossThreadCopy(sortedPrevalentResourcesWithoutUserInteraction),
prevalentResourcesDaysSinceUserInteraction =
crossThreadCopy(prevalentResourcesDaysSinceUserInteraction)] () {

It seems like all three of these Vectors could be WTFMove() to the new thread,
since they are never used again after being constructed.