Bug 182319 - Release assertion in Performance::resourceTimingBufferFullTimerFired when the resource timing buffer is shrunk
Summary: Release assertion in Performance::resourceTimingBufferFullTimerFired when the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-30 20:29 PST by Ryosuke Niwa
Modified: 2018-01-31 13:07 PST (History)
4 users (show)

See Also:


Attachments
Fixes the bug (15.74 KB, patch)
2018-01-30 23:42 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (16.13 KB, patch)
2018-01-31 11:39 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (16.09 KB, patch)
2018-01-31 12:51 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-01-30 20:29:05 PST
When the author script shrinks the resource timing buffer while resourcetimingbufferFull event is being dispatched, we hit a release assertion.
Comment 1 Ryosuke Niwa 2018-01-30 20:29:20 PST
<rdar://problem/36904312>
Comment 2 Ryosuke Niwa 2018-01-30 23:42:59 PST
Created attachment 332746 [details]
Fixes the bug
Comment 3 Chris Dumez 2018-01-31 09:30:16 PST
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 4 Chris Dumez 2018-01-31 10:14:29 PST
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())
Comment 5 Ryosuke Niwa 2018-01-31 11:13:52 PST
(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.
Comment 6 Ryosuke Niwa 2018-01-31 11:29:03 PST
(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).
Comment 7 Ryosuke Niwa 2018-01-31 11:39:29 PST
Created attachment 332788 [details]
Fixes the bug
Comment 8 Chris Dumez 2018-01-31 12:27:58 PST
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();
Comment 9 Ryosuke Niwa 2018-01-31 12:46:12 PST
(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!
Comment 10 Ryosuke Niwa 2018-01-31 12:51:26 PST
Created attachment 332795 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2018-01-31 13:07:42 PST
Comment on attachment 332795 [details]
Patch for landing

Clearing flags on attachment: 332795

Committed r227926: <https://trac.webkit.org/changeset/227926>
Comment 12 WebKit Commit Bot 2018-01-31 13:07:43 PST
All reviewed patches have been landed.  Closing bug.