Summary: | ASSERTION FAILED: m_origin || m_type == CachedResource::MainResource | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||||||
Component: | New Bugs | Assignee: | youenn fablet <youennf> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, darin, dbates, japhet, ryanhaddad, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Ryan Haddad
2016-09-22 16:22:50 PDT
Seems to have started with https://trac.webkit.org/changeset/206255 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> All reviewed patches have been landed. Closing bug. Crash seems to have been solved. Oddly, it was solved before this patch landed, not exactly sure why. |