Bug 222998 – REGRESSION: [BigSur] ASSERT NOT REACHED in WebCore::ResourceLoadPriority WebCore::toResourceLoadPriority

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

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

[Radar WebKit Bug Importer]

<rdar://problem/75236575>

[Ryan Haddad]

This assert was added with https://trac.webkit.org/changeset/274161/webkit

[Ben Nham]

Created attachment 422766 [details]
Patch

[Ben Nham]

Committed r274180 (235099@main): <https://commits.webkit.org/235099@main>

[Ryan Haddad]

*** Bug 223003 has been marked as a duplicate of this bug. ***

[Chris Dumez]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Chris Dumez]

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

[Ben Nham]

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

[Ben Nham]

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.