Release assertion in Performance::resourceTimingBufferFullTimerFired when the resource timing buffer is shrunk

[Ryosuke Niwa]

When the author script shrinks the resource timing buffer while resourcetimingbufferFull event is being dispatched, we hit a release assertion.

[Ryosuke Niwa]

<rdar://problem/36904312>

[Ryosuke Niwa]

Created attachment 332746 [details]
Fixes the bug

[Chris Dumez]

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))

[Chris Dumez]

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())

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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).

[Ryosuke Niwa]

Created attachment 332788 [details]
Fixes the bug

[Chris Dumez]

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();

[Ryosuke Niwa]

(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!

[Ryosuke Niwa]

Created attachment 332795 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 332795 [details]
Patch for landing

Clearing flags on attachment: 332795

Committed r227926: <https://trac.webkit.org/changeset/227926>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.