Bug 181137 - Release assert in addResourceTiming when a cache resource is requested during style recalc
Summary: Release assert in addResourceTiming when a cache resource is requested during...
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: 2017-12-22 14:13 PST by Ryosuke Niwa
Modified: 2018-01-30 13:15 PST (History)
10 users (show)

See Also:


Attachments
Fixes the bug (12.12 KB, patch)
2017-12-22 14:47 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (2.18 MB, application/zip)
2017-12-22 15:47 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.62 MB, application/zip)
2017-12-22 15:56 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (3.22 MB, application/zip)
2017-12-22 16:13 PST, EWS Watchlist
no flags Details
Fixes one more bug (15.41 KB, patch)
2017-12-22 17:23 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (14.96 KB, patch)
2018-01-08 18:37 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (14.77 KB, patch)
2018-01-08 18:48 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (2.15 MB, application/zip)
2018-01-08 19:47 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (2.58 MB, application/zip)
2018-01-08 20:09 PST, EWS Watchlist
no flags Details
Patch for landing (15.94 KB, patch)
2018-01-08 20:24 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (2.16 MB, application/zip)
2018-01-08 21:23 PST, EWS Watchlist
no flags Details
Patch for landing (16.00 KB, patch)
2018-01-08 21:50 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 2017-12-22 14:13:28 PST
When a previously loaded resource is requested during a style resolution, we end up trying to synchronously dispatch resourcetimingbufferfull event.
e.g.
 WebCore: WebCore::EventTarget::dispatchEvent(WebCore::Event&)
 WebCore: WebCore::Performance::addResourceTiming(WebCore::ResourceTiming&&)
   WebCore: WebCore::ResourceTimingInformation::addResourceTiming(WebCore::CachedResource&, WebCore::Document&, WebCore::ResourceTiming&&)
     WebCore: WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type, WebCore::CachedResourceRequest&&, WebCore::CachedResourceLoader::ForPreload, WebCore::CachedResourceLoader::DeferOption)
       WebCore: WebCore::CachedResourceLoader::requestImage(WebCore::CachedResourceRequest&&)
         WebCore: WebCore::CSSImageValue::loadImage(WebCore::CachedResourceLoader&, WebCore::ResourceLoaderOptions const&)
          WebCore: WebCore::StyleCachedImage::load(WebCore::CachedResourceLoader&, WebCore::ResourceLoaderOptions const&)
            WebCore: WebCore::Style::loadPendingImage(WebCore::Document&, WebCore::StyleImage const*, WebCore::Element const*, WebCore::Style::LoadPolicy)
              WebCore: WebCore::Style::loadPendingResources(WebCore::RenderStyle&, WebCore::Document&, WebCore::Element const*)
                WebCore: WebCore::RenderElement::initializeStyle()
                 WebCore: WebCore::RenderTreeUpdater::createRenderer(WebCore::Element&, WebCore::RenderStyle&&)
                   WebCore: WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&)
                     WebCore: WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&)
                       WebCore: WebCore::RenderTreeUpdater::commit(std::__::unique_ptr<WebCore::Style::Update const, std::__::default_delete<WebCore::Style::Update const> >)
                         WebCore: WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)
                           WebCore: WebCore::Document::updateStyleIfNeeded()
                            pruning:  WebCore: WebCore::Document::updateLayout()
Comment 1 Ryosuke Niwa 2017-12-22 14:13:41 PST
<rdar://problem/35666574>
Comment 2 Ryosuke Niwa 2017-12-22 14:47:41 PST
Created attachment 330146 [details]
Fixes the bug
Comment 3 EWS Watchlist 2017-12-22 15:47:22 PST
Comment on attachment 330146 [details]
Fixes the bug

Attachment 330146 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5806447

New failing tests:
http/wpt/resource-timing/rt-performance-extensions.worker.html
http/wpt/resource-timing/rt-performance-extensions.html
Comment 4 EWS Watchlist 2017-12-22 15:47:23 PST
Created attachment 330150 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 EWS Watchlist 2017-12-22 15:56:48 PST
Comment on attachment 330146 [details]
Fixes the bug

Attachment 330146 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5806557

New failing tests:
http/wpt/resource-timing/rt-performance-extensions.worker.html
http/wpt/resource-timing/rt-performance-extensions.html
Comment 6 EWS Watchlist 2017-12-22 15:56:49 PST
Created attachment 330151 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 7 EWS Watchlist 2017-12-22 16:13:01 PST
Comment on attachment 330146 [details]
Fixes the bug

Attachment 330146 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5806564

New failing tests:
http/wpt/resource-timing/rt-performance-extensions.worker.html
http/wpt/resource-timing/rt-performance-extensions.html
Comment 8 EWS Watchlist 2017-12-22 16:13:03 PST
Created attachment 330152 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Ryosuke Niwa 2017-12-22 17:23:52 PST
Created attachment 330154 [details]
Fixes one more bug
Comment 10 Simon Fraser (smfr) 2017-12-22 18:37:59 PST
Comment on attachment 330154 [details]
Fixes one more bug

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

> Source/WebCore/page/Performance.cpp:181
> +    Ref<PerformanceResourceTiming> entry = PerformanceResourceTiming::create(m_timeOrigin, WTFMove(resourceTiming));

auto?

> Source/WebCore/page/Performance.cpp:189
> +        queueEntry(entry.get());

This might read better as enqueueEntry()

> Source/WebCore/page/Performance.cpp:193
> +    if (isResourceTimingBufferFull()) {

Odd that you check m_resourceTimingBufferFull above, but isResourceTimingBufferFull() here.

> Source/WebCore/page/Performance.cpp:215
> +        m_resourceTimingBufferFull = true;

I would like this line explained by a comment.

> Source/WebCore/page/Performance.cpp:221
> +        unsigned remainingBufferSize = m_resourceTimingBufferSize - m_resourceTimingBuffer.size();

I hope that never underflows. Maybe ASSERT(m_resourceTimingBufferSize >= m_resourceTimingBuffer.size())?

> Source/WebCore/page/Performance.cpp:230
> +        for (; i < remainingBufferSize && i < backupBuffer.size(); i++)
> +            m_resourceTimingBuffer.append(backupBuffer[i].copyRef());
> +        for (; i < backupBuffer.size(); i++)
> +            m_backupResourceTimingBuffer.append(backupBuffer[i].copyRef());

Is there a way to do this by moving ranges of vector items?
Comment 11 Ryosuke Niwa 2018-01-08 18:34:10 PST
(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 330154 [details]
> Fixes one more bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330154&action=review
> 
> > Source/WebCore/page/Performance.cpp:181
> > +    Ref<PerformanceResourceTiming> entry = PerformanceResourceTiming::create(m_timeOrigin, WTFMove(resourceTiming));
> 
> auto?

Fixed.

> > Source/WebCore/page/Performance.cpp:189
> > +        queueEntry(entry.get());
> 
> This might read better as enqueueEntry()

Yeah, I think it does but that's not a function I'm adding or modifying so I'd keep it as is for now.

> > Source/WebCore/page/Performance.cpp:193
> > +    if (isResourceTimingBufferFull()) {
> 
> Odd that you check m_resourceTimingBufferFull above, but
> isResourceTimingBufferFull() here.

Actually, the code under if (m_resourceTimingBufferFull) is a dead code since m_shouldUseBackupBuffer is always true whenever m_resourceTimingBufferFull is true. Deleted that branch. That means we also don't need m_resourceTimingBufferFull instance variable so removed that.

> 
> > Source/WebCore/page/Performance.cpp:215
> > +        m_resourceTimingBufferFull = true;
> 
> I would like this line explained by a comment.

This flag has been removed.

> > Source/WebCore/page/Performance.cpp:221
> > +        unsigned remainingBufferSize = m_resourceTimingBufferSize - m_resourceTimingBuffer.size();
> 
> I hope that never underflows. Maybe ASSERT(m_resourceTimingBufferSize >=
> m_resourceTimingBuffer.size())?

Sure, I'd even release-assert that since this is not a performance critical code path.

> > Source/WebCore/page/Performance.cpp:230
> > +        for (; i < remainingBufferSize && i < backupBuffer.size(); i++)
> > +            m_resourceTimingBuffer.append(backupBuffer[i].copyRef());
> > +        for (; i < backupBuffer.size(); i++)
> > +            m_backupResourceTimingBuffer.append(backupBuffer[i].copyRef());
> 
> Is there a way to do this by moving ranges of vector items?

I guess we could do:
unsigned i = 0;
for (auto& item : backupBuffer) {
    if (i < remainingBufferSize)
        m_resourceTimingBuffer.append(item.copyRef());
    else
        m_backupResourceTimingBuffer.append(item.copyRef());
    i++;
}
But I'm not certain if it's cleaner.
Comment 12 Ryosuke Niwa 2018-01-08 18:37:05 PST
Created attachment 330774 [details]
Patch for landing
Comment 13 Ryosuke Niwa 2018-01-08 18:48:53 PST
Created attachment 330775 [details]
Patch for landing
Comment 14 Ryosuke Niwa 2018-01-08 18:49:26 PST
Comment on attachment 330775 [details]
Patch for landing

Wait for EWS.
Comment 15 Joseph Pecoraro 2018-01-08 19:01:17 PST
Looks good to me too@
Comment 16 EWS Watchlist 2018-01-08 19:47:39 PST
Comment on attachment 330775 [details]
Patch for landing

Attachment 330775 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5986367

New failing tests:
http/wpt/resource-timing/rt-performance-extensions.worker.html
http/wpt/resource-timing/rt-performance-extensions.html
Comment 17 EWS Watchlist 2018-01-08 19:47:41 PST
Created attachment 330784 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Ryosuke Niwa 2018-01-08 19:53:50 PST
Oops, no, that wasn't a dead code. It was preventing resourcetimingbufferfull from gettin dispatched multiple times. Restoring the flag.
Comment 19 EWS Watchlist 2018-01-08 20:09:11 PST
Comment on attachment 330775 [details]
Patch for landing

Attachment 330775 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5986783

New failing tests:
http/wpt/resource-timing/rt-performance-extensions.worker.html
Comment 20 EWS Watchlist 2018-01-08 20:09:13 PST
Created attachment 330786 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 21 Ryosuke Niwa 2018-01-08 20:18:16 PST
Comment on attachment 330154 [details]
Fixes one more bug

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

>>> Source/WebCore/page/Performance.cpp:193
>>> +    if (isResourceTimingBufferFull()) {
>> 
>> Odd that you check m_resourceTimingBufferFull above, but isResourceTimingBufferFull() here.
> 
> Actually, the code under if (m_resourceTimingBufferFull) is a dead code since m_shouldUseBackupBuffer is always true whenever m_resourceTimingBufferFull is true. Deleted that branch. That means we also don't need m_resourceTimingBufferFull instance variable so removed that.

Actually, that code was necessary. It was used when resourcetimingbufferfull had fired but the buffer had not been cleared.
Restoring the code with some comments:

    if (m_shouldUseBackupBuffer) {
        // We're waiting for resourceTimingBufferFullTimer to be fired.
        m_backupResourceTimingBuffer.append(WTFMove(entry));
        return;
    }

    if (m_resourceTimingBufferFullFlag) {
        // We fired resourcetimingbufferfull evnet but the author script didn't clear the buffer.
        // Notify performance observers but don't add it to the buffer.
        queueEntry(entry.get());
        return;
    }
Comment 22 Ryosuke Niwa 2018-01-08 20:21:18 PST
(In reply to Ryosuke Niwa from comment #21)
> Comment on attachment 330154 [details]
> Fixes one more bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330154&action=review
> 
> >>> Source/WebCore/page/Performance.cpp:193
> >>> +    if (isResourceTimingBufferFull()) {
> >> 
> >> Odd that you check m_resourceTimingBufferFull above, but isResourceTimingBufferFull() here.
> > 
> > Actually, the code under if (m_resourceTimingBufferFull) is a dead code since m_shouldUseBackupBuffer is always true whenever m_resourceTimingBufferFull is true. Deleted that branch. That means we also don't need m_resourceTimingBufferFull instance variable so removed that.
> 
> Actually, that code was necessary. It was used when resourcetimingbufferfull
> had fired but the buffer had not been cleared.
> Restoring the code with some comments:
> 
>     if (m_shouldUseBackupBuffer) {
>         // We're waiting for resourceTimingBufferFullTimer to be fired.
>         m_backupResourceTimingBuffer.append(WTFMove(entry));
>         return;
>     }

Renamed this instance variable to m_waitingForBackupBufferToBeProcessed instead so that code reads better:
    if (m_waitingForBackupBufferToBeProcessed) {
        m_backupResourceTimingBuffer.append(WTFMove(entry));
        return;
    }
Comment 23 Ryosuke Niwa 2018-01-08 20:24:43 PST
Created attachment 330787 [details]
Patch for landing
Comment 24 EWS Watchlist 2018-01-08 21:23:56 PST
Comment on attachment 330787 [details]
Patch for landing

Attachment 330787 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5988382

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-crash.html
Comment 25 EWS Watchlist 2018-01-08 21:23:58 PST
Created attachment 330791 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 26 Ryosuke Niwa 2018-01-08 21:50:33 PST
Created attachment 330793 [details]
Patch for landing
Comment 27 WebKit Commit Bot 2018-01-09 00:34:38 PST
Comment on attachment 330793 [details]
Patch for landing

Clearing flags on attachment: 330793

Committed r226617: <https://trac.webkit.org/changeset/226617>
Comment 28 WebKit Commit Bot 2018-01-09 00:34:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Chris Dumez 2018-01-30 12:07:18 PST
Comment on attachment 330793 [details]
Patch for landing

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

> Source/WebCore/page/Performance.h:115
> +    Timer m_resourceTimingBufferFullTimer;

Performance is exposed to workers so I do not think this timer is OK. See rdar://problem/36904312. I believe we now run code on the main thread when this timer fires instead of the worker thread.
Comment 30 Chris Dumez 2018-01-30 13:15:53 PST
Comment on attachment 330793 [details]
Patch for landing

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

>> Source/WebCore/page/Performance.h:115
>> +    Timer m_resourceTimingBufferFullTimer;
> 
> Performance is exposed to workers so I do not think this timer is OK. See rdar://problem/36904312. I believe we now run code on the main thread when this timer fires instead of the worker thread.

Nope, looks like I was wrong. The timer does fire on the background thread in case of workers. So it we have a lot of crashes when this timer fires on the main thread (rdar://problem/36904312), however it is not related to workers. Something else is going on.