Bug 162472 - ASSERTION FAILED: m_origin || m_type == CachedResource::MainResource
Summary: ASSERTION FAILED: m_origin || m_type == CachedResource::MainResource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-22 16:22 PDT by Ryan Haddad
Modified: 2016-09-27 00:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.08 KB, patch)
2016-09-23 00:48 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.96 KB, patch)
2016-09-23 10:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (8.86 KB, patch)
2016-09-26 04:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (8.91 KB, patch)
2016-09-26 04:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (2.25 KB, patch)
2016-09-26 11:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2016-09-22 16:22:50 PDT
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
Comment 1 Ryan Haddad 2016-09-22 16:23:18 PDT
Seems to have started with https://trac.webkit.org/changeset/206255
Comment 2 Ryan Haddad 2016-09-22 16:23:45 PDT
<rdar://problem/28431522>
Comment 3 youenn fablet 2016-09-23 00:48:24 PDT
Created attachment 289668 [details]
Patch
Comment 4 youenn fablet 2016-09-23 10:41:53 PDT
Created attachment 289689 [details]
Patch
Comment 5 Darin Adler 2016-09-25 19:12:11 PDT
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.
Comment 6 youenn fablet 2016-09-26 04:13:09 PDT
Created attachment 289816 [details]
Patch for landing
Comment 7 youenn fablet 2016-09-26 04:17:03 PDT
Created attachment 289817 [details]
Patch for landing
Comment 8 youenn fablet 2016-09-26 04:18:02 PDT
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 9 WebKit Commit Bot 2016-09-26 04:49:12 PDT
Comment on attachment 289817 [details]
Patch for landing

Clearing flags on attachment: 289817

Committed r206370: <http://trac.webkit.org/changeset/206370>
Comment 10 WebKit Commit Bot 2016-09-26 04:49:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Ryan Haddad 2016-09-26 09:41:23 PDT
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 12 Darin Adler 2016-09-26 10:01:18 PDT
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.
Comment 13 Alexey Proskuryakov 2016-09-26 10:54:07 PDT
Re-opening, because this assertion still occurs.
Comment 14 youenn fablet 2016-09-26 11:04:25 PDT
Created attachment 289838 [details]
Patch
Comment 15 youenn fablet 2016-09-26 11:05:09 PDT
(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
Comment 16 youenn fablet 2016-09-26 12:43:47 PDT
(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 17 WebKit Commit Bot 2016-09-26 13:21:55 PDT
Comment on attachment 289838 [details]
Patch

Clearing flags on attachment: 289838

Committed r206388: <http://trac.webkit.org/changeset/206388>
Comment 18 WebKit Commit Bot 2016-09-26 13:22:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 youenn fablet 2016-09-27 00:33:01 PDT
Crash seems to have been solved.
Oddly, it was solved before this patch landed, not exactly sure why.