Summary: | Crash in WTF::Atomic<unsigned char>::compareExchangeWeak + 36 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||
Component: | New Bugs | Assignee: | John Wilander <wilander> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, jlewis3, webkit-bug-importer, wilander | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Other | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ryan Haddad
2017-12-08 13:37:03 PST
Created attachment 328864 [details]
Crash log
Created attachment 329144 [details]
Patch
Comment on attachment 329144 [details] 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. Created attachment 329150 [details]
Patch
(In reply to Brent Fulgham from comment #4) > Comment on attachment 329144 [details] > 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? We did when I was using crossThreadCopy(). > > 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. Got it. Fixed. > > 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. Done. Thanks, Brent! Comment on attachment 329150 [details]
Patch
Looks great! r=me
Comment on attachment 329150 [details]
Patch
Thanks again, Brent!
Comment on attachment 329150 [details] Patch Clearing flags on attachment: 329150 Committed r225818: <https://trac.webkit.org/changeset/225818> All reviewed patches have been landed. Closing bug. |