Summary: | Add null checks in ResourceLoader | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, ews-watchlist, ggaren, japhet, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Alex Christensen
2020-07-25 10:01:13 PDT
Created attachment 405229 [details]
Patch
Created attachment 405230 [details]
Patch
Well it doesn't crash any more, but it doesn't finish loading. It prints this out: /Users/.../Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm(258) : void WebCore::WebCoreAVFResourceLoader::startLoading() ERROR: Failed to start load for media at url data:audio/mp3;base64,//uQx Ews failure looks real Created attachment 405343 [details]
Patch
Comment on attachment 405343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405343&action=review > Source/WebCore/ChangeLog:12 > + The original patch for this has a test that would reach this code, but it never finishes loading. Can we do an API test and use a timer to conclude the test (and verify it does not hit some debug assert for instance)? > Source/WebCore/loader/ResourceLoader.cpp:126 > + if (!m_documentLoader || !m_documentLoader->frame()) { Can we keep ASSERT(m_documentLoader)? Comment on attachment 405343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405343&action=review >> Source/WebCore/ChangeLog:12 >> + The original patch for this has a test that would reach this code, but it never finishes loading. > > Can we do an API test and use a timer to conclude the test (and verify it does not hit some debug assert for instance)? I tried that. It didn't trigger the crash before the change. I spent lots of time looking into it. >> Source/WebCore/loader/ResourceLoader.cpp:126 >> + if (!m_documentLoader || !m_documentLoader->frame()) { > > Can we keep ASSERT(m_documentLoader)? No, that would be an invalid assertion. m_documentLoader can be null here. Committed r264990: <https://trac.webkit.org/changeset/264990> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405343 [details]. |