Summary: | Release assertion in Performance::resourceTimingBufferFullTimerFired when the resource timing buffer is shrunk | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
Component: | WebCore Misc. | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, joepeck | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2018-01-30 20:29:05 PST
Created attachment 332746 [details]
Fixes the bug
Comment on attachment 332746 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=332746&action=review Will do a proper review after my meeting. > Source/WebCore/page/Performance.cpp:233 > + for (auto& entry : m_backupResourceTimingBuffer) Wouldn't this work? backupBuffer.appendVector(WTFMove(m_backupResourceTimingBuffer)) Comment on attachment 332746 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=332746&action=review > Source/WebCore/page/Performance.cpp:223 > // Dispatching resourcetimingbufferfull event may have inserted more entries. The line after queues entries from m_backupResourceTimingBuffer but does not seem to clear m_backupResourceTimingBuffer. Is that right? > Source/WebCore/page/Performance.cpp:239 > if (i < remainingBufferSize) { This might be more easily understandable if we used if (!isResourceTimingBufferFull()) (In reply to Chris Dumez from comment #3) > Comment on attachment 332746 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332746&action=review > > Will do a proper review after my meeting. > > > Source/WebCore/page/Performance.cpp:233 > > + for (auto& entry : m_backupResourceTimingBuffer) > > Wouldn't this work? > backupBuffer.appendVector(WTFMove(m_backupResourceTimingBuffer)) Yeah, I realized that after uploading the patch. Fixed. (In reply to Chris Dumez from comment #4) > Comment on attachment 332746 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332746&action=review > > > Source/WebCore/page/Performance.cpp:223 > > // Dispatching resourcetimingbufferfull event may have inserted more entries. > > The line after queues entries from m_backupResourceTimingBuffer but does not > seem to clear m_backupResourceTimingBuffer. Is that right? Oops, fixed. > > Source/WebCore/page/Performance.cpp:239 > > if (i < remainingBufferSize) { > > This might be more easily understandable if we used if > (!isResourceTimingBufferFull()) Sure. Fixed. (In reply to Chris Dumez from comment #3) > Comment on attachment 332746 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332746&action=review > > Will do a proper review after my meeting. > > > Source/WebCore/page/Performance.cpp:233 > > + for (auto& entry : m_backupResourceTimingBuffer) > > Wouldn't this work? > backupBuffer.appendVector(WTFMove(m_backupResourceTimingBuffer)) Actually, this doesn't work because backupBuffer is a WTFMove(m_backupResourceTimingBuffer). Created attachment 332788 [details]
Fixes the bug
Comment on attachment 332788 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=332788&action=review r=me with suggestion. > Source/WebCore/page/Performance.cpp:232 > + for (auto & entry : m_backupResourceTimingBuffer) I think we should be able to do this on 2 lines instead of 3 (and potentially more efficient) using: backupBuffer.appendVector(m_backupResourceTimingBuffer); m_backupResourceTimingBuffer.clear(); (In reply to Chris Dumez from comment #8) > Comment on attachment 332788 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332788&action=review > > r=me with suggestion. > > > Source/WebCore/page/Performance.cpp:232 > > + for (auto & entry : m_backupResourceTimingBuffer) > > I think we should be able to do this on 2 lines instead of 3 (and > potentially more efficient) using: > backupBuffer.appendVector(m_backupResourceTimingBuffer); > m_backupResourceTimingBuffer.clear(); Sure, will fix. Thanks for the review! Created attachment 332795 [details]
Patch for landing
Comment on attachment 332795 [details] Patch for landing Clearing flags on attachment: 332795 Committed r227926: <https://trac.webkit.org/changeset/227926> All reviewed patches have been landed. Closing bug. |