Summary: | REGRESSION: [BigSur] ASSERT NOT REACHED in WebCore::ResourceLoadPriority WebCore::toResourceLoadPriority | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||
Component: | New Bugs | Assignee: | Ben Nham <nham> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, darin, jenner, nham, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | Other | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=222934 | ||||||
Attachments: |
|
Description
Ryan Haddad
2021-03-09 14:54:46 PST
This assert was added with https://trac.webkit.org/changeset/274161/webkit Created attachment 422766 [details]
Patch
Committed r274180 (235099@main): <https://commits.webkit.org/235099@main> *** Bug 223003 has been marked as a duplicate of this bug. *** (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 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 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 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 :) (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. 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. |