Bug 222998 - REGRESSION: [BigSur] ASSERT NOT REACHED in WebCore::ResourceLoadPriority WebCore::toResourceLoadPriority
Summary: REGRESSION: [BigSur] ASSERT NOT REACHED in WebCore::ResourceLoadPriority WebC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Nham
URL:
Keywords: InRadar
: 223003 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-03-09 14:54 PST by Ryan Haddad
Modified: 2021-03-09 20:30 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2021-03-09 15:22 PST, Ben Nham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2021-03-09 14:54:46 PST
Seeing the following assertion failure on Big Sur Debug bots, causing tests to exit early: 

SHOULD NEVER BE REACHED
/Volumes/Data/worker/bigsur-debug/build/Source/WebCore/platform/network/cf/ResourceRequestCFNet.h(62) : WebCore::ResourceLoadPriority WebCore::toResourceLoadPriority(CFURLRequestPriority)
1   0x142581769 WTFCrash
2   0x12602143b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x128207fca WebCore::toResourceLoadPriority(long)
4   0x12820788b WebCore::ResourceRequest::doUpdateResourceRequest()
5   0x12a7c2e67 WebCore::ResourceRequestBase::updateResourceRequest(WebCore::HTTPBodyUpdatePolicy) const
6   0x12a7c2a74 WebCore::ResourceRequestBase::setPriority(WebCore::ResourceLoadPriority)
7   0x12b51da8d WebCore::WebCoreAVFResourceLoader::startLoading()
8   0x126423fee WebCore::MediaPlayerPrivateAVFoundationObjC::shouldWaitForLoadingOfResource(AVAssetResourceLoadingRequest*)
9   0x12646e1c0 -[WebCoreAVFLoaderDelegate resourceLoader:shouldWaitForLoadingOfRequestedResource:]::$_15::operator()()::'lambda'()::operator()() const
10  0x12646e06e WTF::Detail::CallableWrapper<-[WebCoreAVFLoaderDelegate resourceLoader:shouldWaitForLoadingOfRequestedResource:]::$_15::operator()()::'lambda'(), void>::call()
11  0x126036422 WTF::Function<void ()>::operator()() const
12  0x1263f1765 WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'()::operator()() const
13  0x1263f159e WTF::Detail::CallableWrapper<WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'(), void>::call()
14  0x126036422 WTF::Function<void ()>::operator()() const
15  0x12a1fc82f WebCore::TaskDispatcher<WebCore::Timer>::dispatchOneTask()
16  0x12a1fc505 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired()
17  0x12a203e51 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_2::operator()() const
18  0x12a203e0e WTF::Detail::CallableWrapper<WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_2, void>::call()
19  0x126036422 WTF::Function<void ()>::operator()() const
20  0x12607974e WebCore::Timer::fired()
21  0x12a25ffa4 WebCore::ThreadTimers::sharedTimerFiredInternal()
22  0x12a26a481 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
23  0x12a26a42e WTF::Detail::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, void>::call()
24  0x126036422 WTF::Function<void ()>::operator()() const
25  0x12a217a5b WebCore::MainThreadSharedTimer::fired()
26  0x12a2e8176 WebCore::timerFired(__CFRunLoopTimer*, void*)
27  0x7fff2047990d __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
28  0x7fff204793e8 __CFRunLoopDoTimer
29  0x7fff20478f42 __CFRunLoopDoTimers
30  0x7fff2045f57f __CFRunLoopRun
31  0x7fff2045e6ce CFRunLoopRunSpecific

https://build.webkit.org/results/Apple-BigSur-Debug-WK2-Tests/r274171%20(608)/results.html
Comment 1 Radar WebKit Bug Importer 2021-03-09 14:54:55 PST
<rdar://problem/75236575>
Comment 2 Ryan Haddad 2021-03-09 14:56:28 PST
This assert was added with https://trac.webkit.org/changeset/274161/webkit
Comment 3 Ben Nham 2021-03-09 15:22:07 PST
Created attachment 422766 [details]
Patch
Comment 4 Ben Nham 2021-03-09 15:24:46 PST
Committed r274180 (235099@main): <https://commits.webkit.org/235099@main>
Comment 5 Ryan Haddad 2021-03-09 16:28:23 PST
*** Bug 223003 has been marked as a duplicate of this bug. ***
Comment 6 Chris Dumez 2021-03-09 16:29:06 PST
(In reply to Ben Nham from comment #4)
> Committed r274180 (235099@main): <https://commits.webkit.org/235099@main>

Is VeryLow the priority we really want when we don't have a priority set?
Comment 7 Darin Adler 2021-03-09 18:56:04 PST
Comment on attachment 422766 [details]
Patch

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

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:53
>          return ResourceLoadPriority::VeryLow;

Is it OK correct to translate this -1 into VeryLow? Why not Medium? I don’t understand the repercussions.
Comment 8 Darin Adler 2021-03-09 18:56:05 PST
Comment on attachment 422766 [details]
Patch

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

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:53
>          return ResourceLoadPriority::VeryLow;

Is it OK correct to translate this -1 into VeryLow? Why not Medium? I don’t understand the repercussions.
Comment 9 Chris Dumez 2021-03-09 18:57:49 PST
(In reply to Darin Adler from comment #8)
> Comment on attachment 422766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422766&action=review
> 
> > Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:53
> >          return ResourceLoadPriority::VeryLow;
> 
> Is it OK correct to translate this -1 into VeryLow? Why not Medium? I don’t
> understand the repercussions.

Ahah that’s what I asked as well :)
Comment 10 Ben Nham 2021-03-09 19:46:06 PST
(In reply to Darin Adler from comment #8)
> Comment on attachment 422766 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422766&action=review
> 
> > Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:53
> >          return ResourceLoadPriority::VeryLow;
> 
> Is it OK correct to translate this -1 into VeryLow? Why not Medium? I don’t
> understand the repercussions.

(In reply to Chris Dumez from comment #9)
> (In reply to Darin Adler from comment #8)
> > Comment on attachment 422766 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=422766&action=review
> > 
> > > Source/WebCore/platform/network/cf/ResourceRequestCFNet.h:53
> > >          return ResourceLoadPriority::VeryLow;
> > 
> > Is it OK correct to translate this -1 into VeryLow? Why not Medium? I don’t
> > understand the repercussions.
> 
> Ahah that’s what I asked as well :)

From a regression point of view, mapping -1 to VeryLow is fine since that is exactly what we were doing before:

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/network/cf/ResourceRequestCFNet.h?rev=274160#L52

and the overall mapping is now exactly the same as it was before I introduced the FIXME in r252431:

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/network/cf/ResourceRequestCFNet.h?rev=252430#L46

As for what the proper default request priority is, I'm not really sure. It probably should be ResourceLoadPriority::Low based on the fact that this is the default used in both CachedResource::defaultPriorityForResourceType and ResourceRequestBase::m_priority. In practice I don't think it would make any perf difference since the call sites I looked at all set an explicit priority anyway. But I can make a separate cleanup patch tomorrow that adds a ResourceLoadPriority::Default and return that when the CF request priority is unset.
Comment 11 Ben Nham 2021-03-09 20:30:46 PST
Now that I think about it, the existing mapping of CFURLRequestPriority -1 to ResourceLoadPriority::VeryLow makes sense, so I don't think I'm going to change that. The reason is that if you don't set a request priority on the CFURLRequest (meaning CFURLRequestGetRequestPriority returns -1), it actually gets treated as if you had set a priority of 0 (see HTTPConnectionCacheEntry::_prepareNewRequest). So the fact that both -1 and 0 both map to the same value of VeryLow reflects actual behavior.

I could add a new enum value ResourceLoadPriority::Unspecified for this -1 case, which would probably be the best idea if I was starting from scratch. But it doesn't seem like a worthwhile refactoring given that this current mapping of -1 to VeryLow has been the same for years (until I accidentally removed it) and reflects reality.