222998 2021-03-09 14:54:46 -0800 REGRESSION: [BigSur] ASSERT NOT REACHED in WebCore::ResourceLoadPriority WebCore::toResourceLoadPriority 2021-03-09 20:30:46 -0800 1 1 1 Unclassified WebKit New Bugs Other Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=222934 InRadar P2 Normal --- 1 ryanhaddad nham cdumez darin jenner nham webkit-bot-watchers-bugzilla webkit-bug-importer oldest_to_newest 1737783 0 ryanhaddad 2021-03-09 14:54:46 -0800 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 1737784 1 webkit-bug-importer 2021-03-09 14:54:55 -0800 <rdar://problem/75236575> 1737786 2 ryanhaddad 2021-03-09 14:56:28 -0800 This assert was added with https://trac.webkit.org/changeset/274161/webkit 1737796 3 422766 nham 2021-03-09 15:22:07 -0800 Created attachment 422766 Patch 1737801 4 nham 2021-03-09 15:24:46 -0800 Committed r274180 (235099@main): <https://commits.webkit.org/235099@main> 1737845 5 ryanhaddad 2021-03-09 16:28:23 -0800 *** Bug 223003 has been marked as a duplicate of this bug. *** 1737846 6 cdumez 2021-03-09 16:29:06 -0800 (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? 1737894 7 422766 darin 2021-03-09 18:56:04 -0800 Comment on attachment 422766 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. 1737896 8 422766 darin 2021-03-09 18:56:05 -0800 Comment on attachment 422766 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. 1737897 9 cdumez 2021-03-09 18:57:49 -0800 (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 :) 1737905 10 nham 2021-03-09 19:46:06 -0800 (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. 1737915 11 nham 2021-03-09 20:30:46 -0800 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. 422766 2021-03-09 15:22:07 -0800 2021-03-09 15:22:07 -0800 Patch bug-222998-20210309152206.patch text/plain 1628 nham U3VidmVyc2lvbiBSZXZpc2lvbjogMjc0MTc5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDQ0OWRjOTkxZjhkZGU4 ZmJkOGQ5YjMzNGJjNzhmODg1ZDM5NDI0Yy4uZTJmMDEyZmQ3YzU1Y2JhNzM1NzJiZTMzNDlkNjUy MDdhZjY2YjcxZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDIxLTAzLTA5ICBCZW4g TmhhbSAgPG5oYW1AYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT046IFtCaWdTdXJdIEFT U0VSVCBOT1QgUkVBQ0hFRCBpbiBXZWJDb3JlOjpSZXNvdXJjZUxvYWRQcmlvcml0eSBXZWJDb3Jl Ojp0b1Jlc291cmNlTG9hZFByaW9yaXR5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0yMjI5OTgKKworICAgICAgICBVbnJldmlld2VkLCBmaXggdGhlIGRl YnVnIGFzc2VydCBjYXVzZWQgYnkgcjI3NDE2MS4KKworICAgICAgICBDRlVSTFJlcXVlc3RHZXRS ZXF1ZXN0UHJpb3JpdHkgY2FuIHJldHVybiAtMSBmb3IgcmVxdWVzdHMgd2l0aCBubyBwcmlvcml0 eSBzZXQgKGUuZy4gc29tZQorICAgICAgICByZXF1ZXN0cyBmcm9tIHRoZSBtZWRpYSBzdGFjayku IFdlIG5lZWQgdG8gaGFuZGxlIHRoaXMgY2FzZSB3aGVuIHRyYW5zZm9ybWluZyBhCisgICAgICAg IENGVVJMUmVxdWVzdFByaW9yaXR5IHRvIGEgUmVzb3VyY2VMb2FkUHJpb3JpdHkuCisKKyAgICAg ICAgKiBwbGF0Zm9ybS9uZXR3b3JrL2NmL1Jlc291cmNlUmVxdWVzdENGTmV0Lmg6CisgICAgICAg IChXZWJDb3JlOjp0b1Jlc291cmNlTG9hZFByaW9yaXR5KToKKwogMjAyMS0wMy0wOSAgRXJpYyBD YXJsc29uICA8ZXJpYy5jYXJsc29uQGFwcGxlLmNvbT4KIAogICAgICAgICBbR1BVIFByb2Nlc3Nd IE1TRSB2aWRlb3MgYXJlIHVuYWJsZSB0byBBaXJQbGF5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9uZXR3b3JrL2NmL1Jlc291cmNlUmVxdWVzdENGTmV0LmggYi9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL2NmL1Jlc291cmNlUmVxdWVzdENGTmV0LmgKaW5kZXgg ZTY2ZmExOGQxNWE5YWNiNmRlNzYwMjJlOTNjMjg5MjdjMzg4MjkxNS4uYmNjNjIyODY2MGUzODY4 ZWM3YzFhNmI0N2RlZTFlZDcxYmYwM2Q5YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vbmV0d29yay9jZi9SZXNvdXJjZVJlcXVlc3RDRk5ldC5oCisrKyBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL25ldHdvcmsvY2YvUmVzb3VyY2VSZXF1ZXN0Q0ZOZXQuaApAQCAtNDgsNiAr NDgsNyBAQCBDRlVSTFJlcXVlc3RSZWYgY2ZVUkxSZXF1ZXN0KGNvbnN0IFJlc291cmNlUmVxdWVz dCYpOwogaW5saW5lIFJlc291cmNlTG9hZFByaW9yaXR5IHRvUmVzb3VyY2VMb2FkUHJpb3JpdHko Q0ZVUkxSZXF1ZXN0UHJpb3JpdHkgcHJpb3JpdHkpCiB7CiAgICAgc3dpdGNoIChwcmlvcml0eSkg eworICAgIGNhc2UgLTE6CiAgICAgY2FzZSAwOgogICAgICAgICByZXR1cm4gUmVzb3VyY2VMb2Fk UHJpb3JpdHk6OlZlcnlMb3c7CiAgICAgY2FzZSAxOgo=