Bug 162472 – ASSERTION FAILED: m_origin || m_type == CachedResource::MainResource

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

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

[Ryan Haddad]

Seems to have started with https://trac.webkit.org/changeset/206255

[Ryan Haddad]

<rdar://problem/28431522>

[youenn fablet]

Created attachment 289668 [details]
Patch

[youenn fablet]

Created attachment 289689 [details]
Patch

[Darin Adler]

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.

[youenn fablet]

Created attachment 289816 [details]
Patch for landing

[youenn fablet]

Created attachment 289817 [details]
Patch for landing

[youenn fablet]

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.

[WebKit Commit Bot]

Comment on attachment 289817 [details]
Patch for landing

Clearing flags on attachment: 289817

Committed r206370: <http://trac.webkit.org/changeset/206370>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Darin Adler]

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.

[Alexey Proskuryakov]

Re-opening, because this assertion still occurs.

[youenn fablet]

Created attachment 289838 [details]
Patch

[youenn fablet]

(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

[youenn fablet]

(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)

[WebKit Commit Bot]

Comment on attachment 289838 [details]
Patch

Clearing flags on attachment: 289838

Committed r206388: <http://trac.webkit.org/changeset/206388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Crash seems to have been solved.
Oddly, it was solved before this patch landed, not exactly sure why.