Encountered on iOS with API test WebKit1.MemoryCacheAddImageToCache https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20%28Tests%29/builds/221 UNEXPECTEDLY EXITED WebKit1.MemoryCacheAddImageToCache ASSERTION FAILED: m_origin || m_type == CachedResource::MainResource /Volumes/Data/slave/ios-simulator-10-debug/build/Source/WebCore/loader/cache/CachedResource.cpp(153) : WebCore::CachedResource::CachedResource(WebCore::CachedResourceRequest &&, WebCore::CachedResource::Type, WebCore::SessionID) 1 0x1038193ad WTFCrash 2 0x1081fee20 WebCore::CachedResource::CachedResource(WebCore::CachedResourceRequest&&, WebCore::CachedResource::Type, WebCore::SessionID) 3 0x1081f249a WebCore::CachedImage::CachedImage(WebCore::URL const&, WebCore::Image*, WebCore::CachedImage::CacheBehaviorType, WebCore::SessionID) 4 0x1081f26a3 WebCore::CachedImage::CachedImage(WebCore::URL const&, WebCore::Image*, WebCore::CachedImage::CacheBehaviorType, WebCore::SessionID) 5 0x10996ecb2 WebCore::MemoryCache::addImageToCache(WTF::RetainPtr<CGImage*>&&, WebCore::URL const&, WTF::String const&) 6 0x115daa1c3 +[WebCache addImageToCache:forURL:forFrame:] 7 0x115daa07e +[WebCache addImageToCache:forURL:] 8 0x1026f831e TestWebKitAPI::WebKit1_MemoryCacheAddImageToCache_Test::TestBody() 9 0x1027e975a testing::Test::Run() 10 0x1027ea58d testing::internal::TestInfoImpl::Run() 11 0x1027eb58d testing::TestCase::Run() 12 0x1027f1ccb testing::internal::UnitTestImpl::RunAllTests() 13 0x1027f1949 testing::UnitTest::Run() 14 0x10276767c TestWebKitAPI::TestsController::run(int, char**) 15 0x1027b9775 main 16 0x11415668d start
Seems to have started with https://trac.webkit.org/changeset/206255
<rdar://problem/28431522>
Created attachment 289668 [details] Patch
Created attachment 289689 [details] Patch
Comment on attachment 289689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289689&action=review > Source/WebCore/loader/cache/CachedResource.cpp:144 > + , m_loading(false) This line is not needed. > Source/WebCore/loader/cache/CachedResource.h:97 > + // Constructor to use by default for all resources I don’t think this comment is needed; I guess maybe the goal here is to make clear the difference between this constructor and the other, but I don’t think as currently worded that this makes that any clearer. > Source/WebCore/loader/cache/CachedResource.h:282 > + // Constructor that can be used when creating already loaded resources I don’t think the phrase “creating already loaded resources” is quite right. If the resource is “already loaded” then I don’t think we can “create” it. Maybe we are using the word “resource” to mean two different things. Maybe this: // Constructor for use when creating a CachedResource for an already-loaded resource. > Source/WebCore/loader/cache/CachedResource.h:305 > + void init(); I don’t really love the name “init” for a function; it’s an abbreviation and we normally prefer to avoid those in WebKIt code. We could call this just “initialize” but I think that given what it actually does, only a small part of the initialization that happens to be shared between two constructors, I would call it finishInitialization or finishConstructing or something more like that.
Created attachment 289816 [details] Patch for landing
Created attachment 289817 [details] Patch for landing
Thanks for the review. (In reply to comment #5) > Comment on attachment 289689 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289689&action=review > > > Source/WebCore/loader/cache/CachedResource.cpp:144 > > + , m_loading(false) > > This line is not needed. Removed. > > Source/WebCore/loader/cache/CachedResource.h:97 > > + // Constructor to use by default for all resources > > I don’t think this comment is needed; I guess maybe the goal here is to make > clear the difference between this constructor and the other, but I don’t > think as currently worded that this makes that any clearer. Removed. > > Source/WebCore/loader/cache/CachedResource.h:282 > > + // Constructor that can be used when creating already loaded resources > > I don’t think the phrase “creating already loaded resources” is quite right. > If the resource is “already loaded” then I don’t think we can “create” it. > Maybe we are using the word “resource” to mean two different things. Maybe > this: > > // Constructor for use when creating a CachedResource for an already-loaded > resource. I reworded to: "CachedResource constructor that may be used when the CachedResource can already be filled with response data." > > > Source/WebCore/loader/cache/CachedResource.h:305 > > + void init(); > > I don’t really love the name “init” for a function; it’s an abbreviation and > we normally prefer to avoid those in WebKIt code. > > We could call this just “initialize” but I think that given what it actually > does, only a small part of the initialization that happens to be shared > between two constructors, I would call it finishInitialization or > finishConstructing or something more like that. Changed to finishRequestInitialization since it is doing that.
Comment on attachment 289817 [details] Patch for landing Clearing flags on attachment: 289817 Committed r206370: <http://trac.webkit.org/changeset/206370>
All reviewed patches have been landed. Closing bug.
It appears that WebKit1.MemoryCacheAddImageToCache is still failing as of r206376, but now on a different line: https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20(Tests)/builds/287 ASSERTION FAILED: m_origin || m_type == CachedResource::MainResource /Volumes/Data/slave/ios-simulator-10-debug/build/Source/WebCore/loader/cache/CachedResource.cpp(131) : WebCore::CachedResource::CachedResource(WebCore::CachedResourceRequest &&, WebCore::CachedResource::Type, WebCore::SessionID) 1 0x10bddf46d WTFCrash 2 0x1107e5abe WebCore::CachedResource::CachedResource(WebCore::CachedResourceRequest&&, WebCore::CachedResource::Type, WebCore::SessionID) 3 0x1107d927a WebCore::CachedImage::CachedImage(WebCore::URL const&, WebCore::Image*, WebCore::CachedImage::CacheBehaviorType, WebCore::SessionID) 4 0x1107d94e3 WebCore::CachedImage::CachedImage(WebCore::URL const&, WebCore::Image*, WebCore::CachedImage::CacheBehaviorType, WebCore::SessionID) 5 0x111f57152 WebCore::MemoryCache::addImageToCache(WTF::RetainPtr<CGImage*>&&, WebCore::URL const&, WTF::String const&) 6 0x11e38d023 +[WebCache addImageToCache:forURL:forFrame:] 7 0x11e38cede +[WebCache addImageToCache:forURL:] 8 0x10acb0d2e TestWebKitAPI::WebKit1_MemoryCacheAddImageToCache_Test::TestBody() 9 0x10ada25fa testing::Test::Run() 10 0x10ada342d testing::internal::TestInfoImpl::Run() 11 0x10ada442d testing::TestCase::Run() 12 0x10adaab6b testing::internal::UnitTestImpl::RunAllTests() 13 0x10adaa7e9 testing::UnitTest::Run() 14 0x10ad2008c TestWebKitAPI::TestsController::run(int, char**) 15 0x10ad72615 main 16 0x11c73968d start
Comment on attachment 289817 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=289817&action=review > Source/WebCore/loader/cache/CachedResource.h:338 > + unsigned m_preloadResult { PreloadNotReferenced }; Missed this in my initial review. The type here should be changed from unsigned to PreloadResult.
Re-opening, because this assertion still occurs.
Created attachment 289838 [details] Patch
(In reply to comment #12) > Comment on attachment 289817 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289817&action=review > > > Source/WebCore/loader/cache/CachedResource.h:338 > > + unsigned m_preloadResult { PreloadNotReferenced }; > > Missed this in my initial review. The type here should be changed from > unsigned to PreloadResult. Fixed in latest patch
(In reply to comment #14) > Created attachment 289838 [details] > Patch Patch is kept to minimal although some additional constructor clean-up could be done (unused constructor, unneeded constructor parameter, unneeded function calls in constructor)
Comment on attachment 289838 [details] Patch Clearing flags on attachment: 289838 Committed r206388: <http://trac.webkit.org/changeset/206388>
Crash seems to have been solved. Oddly, it was solved before this patch landed, not exactly sure why.