Bug 97055 – Reland "Add in-place reload behavior to ImagesEnabled setting" with optimizations

• Component: New Bugs
• Status: RESOLVED FIXED

[Bo Liu]

Reland "Add in-place reload behavior to ImagesEnabled setting" with optimizations

[Bo Liu]

Created attachment 164642 [details]
Patch

[Bo Liu]

Comment on attachment 164642 [details]
Patch

Waiting for chromium m23 branch before landing with cq.

[Adam Barth]

You wrote in the Chromium bug tracker that there's still a 1.3% regression on bloat-http.  Can we implement this feature without introducing a regression?

[Adam Barth]

http://code.google.com/p/chromium/issues/detail?id=150021

[Bo Liu]

(In reply to comment #3)
> You wrote in the Chromium bug tracker that there's still a 1.3% regression on bloat-http.  Can we implement this feature without introducing a regression?

Unfortunately, my naive answer is probably not.

I said in the crbug that most of the regression came from additional calls to WebPermissionClient::allowImage in default image load path. This new patch gets rid of the unnecessary duplicated calls, ie clear winners. The only remaining calls are in CachedResourceLoader::determineRevalidationPolicy and CachedImage::load.

I tried getting rid of the second call by caching the result of allowImage in CachedImage. This hack involved adding two bools (without :1) to CachedImage and did get rid of the second call without breaking any layout tests, but it performs worse in bloat-http (4% vs no patch).

At that point I stopped and figured the regression is probably caused by the overall logic change introduced in this patch.

Writing this comment made me thought of two more things to try but I'm not hopeful:
Retry the hack above with bool:1
Getting rid of the instance variables in CachedResourceLoader and read from Settings directly.

[Adam Barth]

Sorry for not understanding fully.  Is the regression in memory usage or in load time?

[Adam Barth]

Comment on attachment 164642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164642&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:536
> +    // Do not load from cache if images are not enabled. The load for this image will be blocked
> +    // in CachedImage::load.
> +    if (existingResource->type() == CachedResource::ImageResource && !clientAllowsImage(existingResource->url()))
> +        return Reload;

This check doesn't seem right.  Why do we need to call clientAllowsImage again here?  It seems like it should be a property of the image whether its load is being deferred.

[Adam Barth]

Comment on attachment 164642 [details]
Patch

I don't think we want to slow down page load time by 1.3% to support this marginal feature.  We should be able to come up with a design that doesn't have any measurable performance penalty.

[Bo Liu]

(In reply to comment #6)
> Sorry for not understanding fully.  Is the regression in memory usage or in load time?

The regression is in load time. Memory usage should not be affected by this patch in the normal load path.

(In reply to comment #7)
> (From update of attachment 164642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164642&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:536
> > +    // Do not load from cache if images are not enabled. The load for this image will be blocked
> > +    // in CachedImage::load.
> > +    if (existingResource->type() == CachedResource::ImageResource && !clientAllowsImage(existingResource->url()))
> > +        return Reload;
> 
> This check doesn't seem right.  Why do we need to call clientAllowsImage again here?  It seems like it should be a property of the image whether its load is being deferred.

Need a check here so that we will also block image load from cache. I haven't verified this but I think an image from cache will have stillNeedsLoad false load is not called. This is how loadImagesAutomatically allows loading images from memory cache.

(In reply to comment #8)
> (From update of attachment 164642 [details])
> I don't think we want to slow down page load time by 1.3% to support this marginal feature.  We should be able to come up with a design that doesn't have any measurable performance penalty.

I'll try the two changes I mentioned first and get back to you.

[Adam Barth]

> > (From update of attachment 164642 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=164642&action=review
> > 
> > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:536
> > > +    // Do not load from cache if images are not enabled. The load for this image will be blocked
> > > +    // in CachedImage::load.
> > > +    if (existingResource->type() == CachedResource::ImageResource && !clientAllowsImage(existingResource->url()))
> > > +        return Reload;
> > 
> > This check doesn't seem right.  Why do we need to call clientAllowsImage again here?  It seems like it should be a property of the image whether its load is being deferred.
> 
> Need a check here so that we will also block image load from cache. I haven't verified this but I think an image from cache will have stillNeedsLoad false load is not called. This is how loadImagesAutomatically allows loading images from memory cache.

Do we actually need to block images loaded from the MemoryCache?  Presumably flipping this setting isn't going to effect <img> element that are already present in a document.

I wonder if any clients of this setting really depend on blocking images that are already in the MemoryCache...  If the do, perhaps we should just flush the memory cache when this setting changes value.

If I had to guess, I'd guess that this is the line of code that's causing the performance problem.

[Bo Liu]

(In reply to comment #10)
> Do we actually need to block images loaded from the MemoryCache?  Presumably flipping this setting isn't going to effect <img> element that are already present in a document.
There was the comment in Settings.h explicitly stating that it should block cache loads as well. I guess for chromium, this is to prevent the situation that siteA and siteB load the same image but images from only one of the domain is blocked; if they somehow end up in the same webkit instance, then it could appear as image blocking is broken.

> I wonder if any clients of this setting really depend on blocking images that are already in the MemoryCache...  If the do, perhaps we should just flush the memory cache when this setting changes value.

I'll try this as well and see what happens.
> 
> If I had to guess, I'd guess that this is the line of code that's causing the performance problem.

[Bo Liu]

(In reply to comment #10)
> I wonder if any clients of this setting really depend on blocking images that are already in the MemoryCache...  If the do, perhaps we should just flush the memory cache when this setting changes value.

Quick update: I tried to implement this first but could not get the test below to pass. The reason is the test never flips the ImagesEnabled setting, instead only sets the client to ignore the setting and start returning false from allowImage call.

LayoutTests/platform/chromium/permissionclient/image-permissions.html

[Bo Liu]

Created attachment 164794 [details]
shouldDeferImageLoad check hack. Not for commit

[Bo Liu]

Comment on attachment 164794 [details]
shouldDeferImageLoad check hack. Not for commit

Not for commit

[Bo Liu]

(In reply to comment #13)
> Created an attachment (id=164794) [details]
> shouldDeferImageLoad check hack. Not for commit

This was a bit surprising result so uploading.

With this patch, the bloat-http test runs 0.4% faster. I thought this was a fluke so I closed most programs on my desktop and reran the tests, 3 times (ie 30 iterations) each. Also for reference, without the bool:1 in CachedImage, the patch is 1.1% slower.

The hack is adding 2 bools into CachedImage. They are set in CachedResourceLoader::determineRevalidationPolicy and used in CachedImage::load to avoid a single call to allowImage. The complete logic of CachedResourceLoader::shouldDeferImageLoad is duplciated in CachedResourceLoader::determineRevalidationPolicy though.

[Adam Barth]

+eric, who might be interested in how sensitive this benchmark is to CacheImage performance.  Independent of this work, there might be opportunities for performance improvements here.

[Bo Liu]

I'm really baffled at how bool :1 works. I just tried master (without this patch), and add :1 to the only bool in CachedImage and the benchmark runs 1.6% slower.

Now getting back to patch improvements that can possibly be committed...

[Eric Seidel (no email)]

Comment on attachment 164794 [details]
shouldDeferImageLoad check hack. Not for commit

View in context: https://bugs.webkit.org/attachment.cgi?id=164794&action=review

> Source/WebCore/loader/cache/CachedImage.h:118
> +    bool m_shouldPaintBrokenImage : 1;
> +    bool m_skipNextShouldDeferImageCheck : 1;
> +    bool m_shouldDeferImageResult : 1;

These are unsafe!  MSVC uses a signed bool and will do the wrong thing here!  These need to be unsigned or you'll have strange bugs on Windows.  We *really* need to add this to the style-checker pronto.

[Eric Seidel (no email)]

Comment on attachment 164794 [details]
shouldDeferImageLoad check hack. Not for commit

View in context: https://bugs.webkit.org/attachment.cgi?id=164794&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.h:165
>      bool m_autoLoadImages : 1;
> +    bool m_imagesEnabled : 1;
>      bool m_allowStaleResources : 1;

Maybe things have changed... but back when we first brought the Apple Win port up, bool : 1 didn't work right on MSVC.  Last I knew that hadn't changed.  I'll CC some Windows folsk to confirm.

[Eric Seidel (no email)]

are "bool" bitfields OK in modern MSVC?  I thought they still did the wrong thing?  (caused msvc to only store the sign and not the actual value.)

[Eric Seidel (no email)]

I'm interested in playing with this bloat-http test when I'm back from vacation.

[Adam Barth]

We use bool : 1 a lot in Settings.  I hope it works...

[Eric Seidel (no email)]

(In reply to comment #22)
> We use bool : 1 a lot in Settings.  I hope it works...

Sorry.  I'm remembering incorrectly.  It's "enum" which MSVC treats as signed and gets wrong.

bool should be fine!

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/74937 is the kind of bug I was remembering.  Probably not the exact example (since it doesn't have much explanation), but you can see the general idea.

[Bo Liu]

Adam:

I found that CachedImage::load is called twice for each CachedResourceLoader::requestImage call that is not loaded from cache, once in loadResource/revalidateResource once in requestImage.

If I remove the load call in loadResource/revalidateResource, then it brings the patch performance up to before the patch: 0.2% regression which could just be variation in the benchmark. But this is a case of finding existing bad code to improve performance then regressing again in the patch.

I also noticed CachedResourceLoader::requestUserCSSStyleSheet does not call requestResource, but has its own code to handle caching and such. Do you think it's worthwhile to try something like that for requestImage to get rid of the duplicated allowImage call? I think if we do this refactor, then CachedImage should no longer need to override the load method.

[Adam Barth]

> If I remove the load call in loadResource/revalidateResource, then it brings the patch performance up to before the patch: 0.2% regression which could just be variation in the benchmark. But this is a case of finding existing bad code to improve performance then regressing again in the patch.

That's ok with me.  We can work to further optimize the performance in a later patch.

> I also noticed CachedResourceLoader::requestUserCSSStyleSheet does not call requestResource, but has its own code to handle caching and such. Do you think it's worthwhile to try something like that for requestImage to get rid of the duplicated allowImage call? I think if we do this refactor, then CachedImage should no longer need to override the load method.

Sure, we can try that.  Another option is to land the version of the patch without the (observable) regression and then look at that optimization in a subsequent patch.

[Bo Liu]

Created attachment 164988 [details]
Do not call load for image in loadResource/revalidateResource

[Bo Liu]

(In reply to comment #27)
> Created an attachment (id=164988) [details]
> Do not call load for image in loadResource/revalidateResource

I thought this would be good enough to land, but I synced webkit/chromium tree and ran bloat-http again and it showed 1.7% regression again. More confusingly, Patch 1 on the new tip of tree has only 0.1% regression. So removing the load call actually hurt performance, possibly due to the condition check?

Should I attempt the refactor?

[Adam Barth]

> Should I attempt the refactor?

Sure...  Maybe there's enough noise in this benchmark that it's hard to measure what's going on?

[Bo Liu]

Comment on attachment 164988 [details]
Do not call load for image in loadResource/revalidateResource

Have something in the works that is definitely less hacky and *should* perform the same as this.

[Bo Liu]

Created attachment 165015 [details]
Move check logic into CachedImage::load and remove load call at end of requestImage

[Bo Liu]

(In reply to comment #31)
> Created an attachment (id=165015) [details]
> Move check logic into CachedImage::load and remove load call at end of requestImage

This is similar to last one except instead of removing load call in requestResource/revalidateResource, the load in requestImage is removed. This brings requestImage closer to other request* methods.

Performance-wise, regression is under 1%. I think this is the version that should be landed.

In terms of getting rid of the duplicated allowImage call, I tried adding a bool to ResourceLoaderOptions, but the regression caused by initializing this extra bool everywhere else outweighs getting rid of one allowImage call.

I tried the refactor I mentioned, but stopped half way, because even if no code is duplicated, it would make the load call path way more complicated, and that alone would probably offset getting rid of that allowImage call.

[Bo Liu]

Comment on attachment 165015 [details]
Move check logic into CachedImage::load and remove load call at end of requestImage

Seem to have broken an android test downstream. Will investigate a bit.

[Bo Liu]

Comment on attachment 165015 [details]
Move check logic into CachedImage::load and remove load call at end of requestImage

Breaks image loading from cache. Apparently images are special that load needs to be called even when loading directly from cache.

[Bo Liu]

Created attachment 165168 [details]
Add bool parameter to requestResource to avoid duplicated allowImage calls. Refactored CachedResource::load call out of loadResource/revalidateResource to end of requestResource.

[Bo Liu]

(In reply to comment #35)
> Created an attachment (id=165168) [details]
> Add bool parameter to requestResource to avoid duplicated allowImage calls. Refactored CachedResource::load call out of loadResource/revalidateResource to end of requestResource.

Performance on bloat-http should be on par or better than before patch based on my local testing. The refactored logic is not 100% identical to before in moving the load call from loadResource/revalidateResource to end of requestResource, but they seem to be ok by running through some loader layout tests, but please double check.

Style-wise, the autoLoadImages check is in CachedImage::load, but clientDefersImage check is in CachedResourceLoader. This is for performance reasons only. The fall through in the switch statement in requestResource is for performance too.

Please review!

[Build Bot]

Comment on attachment 165168 [details]
Add bool parameter to requestResource to avoid duplicated allowImage calls. Refactored CachedResource::load call out of loadResource/revalidateResource to end of requestResource.

Attachment 165168 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13953718

[Bo Liu]

Created attachment 165173 [details]
Move check logic into CachedImage::load and remove load call at end of requestImage

[Bo Liu]

(In reply to comment #38)
> Created an attachment (id=165173) [details]
> Move check logic into CachedImage::load and remove load call at end of requestImage

Damn it! webkit-patch on tot seem to have problems.

Description should be: Remove unused parameters in loadResource/revalidateResource (Thank you clang!)

[WebKit Review Bot]

Comment on attachment 165173 [details]
Move check logic into CachedImage::load and remove load call at end of requestImage

Attachment 165173 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13916755

New failing tests:
WebFilterOperationsTest.saveAndRestore
fast/html/object-image-nested-fallback.html

[Build Bot]

Comment on attachment 165173 [details]
Move check logic into CachedImage::load and remove load call at end of requestImage

Attachment 165173 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13950770

New failing tests:
fast/html/object-image-nested-fallback.html

[Bo Liu]

Created attachment 165236 [details]
Fix condition for whether to call load

[Adam Barth]

Comment on attachment 165236 [details]
Fix condition for whether to call load

View in context: https://bugs.webkit.org/attachment.cgi?id=165236&action=review

This looks great.  Only one nit-picky comment below.

> Source/WebCore/loader/cache/CachedResourceLoader.h:129
> +    CachedResourceHandle<CachedResource> requestResource(CachedResource::Type, ResourceRequest&, const String& charset, const ResourceLoaderOptions&, ResourceLoadPriority = ResourceLoadPriorityUnresolved, bool isPreload = false, bool imageDeferredByClient = false);

Rather than add Boolean parameter, we prefer to add enum parameters, like ResourceLoadPriority.  That makes things much easier to read at the call site.  I realize that isPreload is already using a Boolean, but we might want to convert that to a two-value enum in a followup patch.

[Bo Liu]

Created attachment 165407 [details]
Use enum instead of bool.

[Adam Barth]

Comment on attachment 165407 [details]
Use enum instead of bool.

Thanks!

[WebKit Review Bot]

Comment on attachment 165407 [details]
Use enum instead of bool.

Clearing flags on attachment: 165407

Committed r129388: <http://trac.webkit.org/changeset/129388>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

This appears to have broken the FrameMIMETypePNG WK2 API test at least on the EFL port: <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6302/steps/API%20tests/logs/stdio>

[Simon Fraser (smfr)]

This seems to be causing a new assertion when running fast/tokenizer/image-empty-crash.html

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef

VM Regions Near 0xbbadbeef:
--> 
    __TEXT                 0000000101b3c000-0000000101b3d000 [    4K] r-x/rwx SM=COW  /Volumes/VOLUME/*/WebKit2.framework/WebProcess.app/Contents/MacOS/WebProcess

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010509f263 WebCore::SubresourceLoader::didFinishLoading(double) + 307 (SubresourceLoader.cpp:293)
1   com.apple.WebCore             	0x0000000104e7f4b5 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 53 (ResourceLoader.cpp:442)
2   com.apple.WebCore             	0x0000000104e7c0ca -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] + 186 (ResourceHandleMac.mm:861)
3   com.apple.Foundation          	0x00007fff96686f58 __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke_0 + 28
4   com.apple.Foundation          	0x00007fff96686e9c -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] + 227
5   com.apple.Foundation          	0x00007fff96686d98 -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] + 63

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r129389%20(1400)/results.html

[Adam Barth]

Bo, how to you feel about rolling out this patch?  We'd prefer not to cause failures on the bot.

[Chris Dumez]

Seems to cause assertions on EFL build bots:
crash log for DumpRenderTree (pid 11106):
STDOUT: <empty>
STDERR: ASSERTION FAILED: size == 0 || size >= m_encodedSize
STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebCore/loader/cache/CachedResource.cpp(523) : void WebCore::CachedResource::setEncodedSize(unsigned int)
STDERR: 1   0x7fe263acdc04 WebCore::CachedResource::setEncodedSize(unsigned int)
STDERR: 2   0x7fe263ac8f4f WebCore::CachedImage::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool)
STDERR: 3   0x7fe263a8f6ef WebCore::SubresourceLoader::sendDataToResource(char const*, int)
STDERR: 4   0x7fe263a8f4b0 WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, bool)
STDERR: 5   0x7fe263a8aa66 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
STDERR: 6   0x7fe264581ee2
STDERR: 7   0x7fe25c333765
STDERR: 8   0x7fe25c3488cd g_simple_async_result_complete
STDERR: 9   0x7fe25c348968
STDERR: 10  0x7fe25c67ae53 g_main_context_dispatch
STDERR: 11  0x7fe260afd23e
STDERR: 12  0x7fe260af77b1
STDERR: 13  0x7fe260af8245
STDERR: 14  0x7fe260af8547 ecore_main_loop_begin
STDERR: 15  0x479cc1
STDERR: 16  0x479e9f
STDERR: 17  0x47a4f8 main
STDERR: 18  0x7fe25f1a776d __libc_start_main
STDERR: 19  0x467999

[Bo Liu]

(In reply to comment #50)
> Bo, how to you feel about rolling out this patch?  We'd prefer not to cause failures on the bot.

Reproduced image-empty-crash.html in chromium and can't see an obvious quick fix. And the other error reports are rolling in...

I guess should revert for now.

[WebKit Review Bot]

Re-opened since this is blocked by 97477

[Bo Liu]

Created attachment 165472 [details]
Fix crash in image-empty-crash.html: was missing stillNeedsLoad check in reloadImagesIfNotDeferred. Other crashes are probably due to same mistake, but haven't checked

[Bo Liu]

(In reply to comment #48)
> This appears to have broken the FrameMIMETypePNG WK2 API test at least on the EFL port: <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6302/steps/API%20tests/logs/stdio>

This is fixed in Patch in attachment 165472 [details]

[Bo Liu]

(In reply to comment #51)
> Seems to cause assertions on EFL build bots:
> crash log for DumpRenderTree (pid 11106):
> STDOUT: <empty>
> STDERR: ASSERTION FAILED: size == 0 || size >= m_encodedSize
> STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebCore/loader/cache/CachedResource.cpp(523) : void WebCore::CachedResource::setEncodedSize(unsigned int)
> STDERR: 1   0x7fe263acdc04 WebCore::CachedResource::setEncodedSize(unsigned int)
> STDERR: 2   0x7fe263ac8f4f WebCore::CachedImage::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool)
> STDERR: 3   0x7fe263a8f6ef WebCore::SubresourceLoader::sendDataToResource(char const*, int)
> STDERR: 4   0x7fe263a8f4b0 WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, bool)
> STDERR: 5   0x7fe263a8aa66 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
> STDERR: 6   0x7fe264581ee2
> STDERR: 7   0x7fe25c333765
> STDERR: 8   0x7fe25c3488cd g_simple_async_result_complete
> STDERR: 9   0x7fe25c348968
> STDERR: 10  0x7fe25c67ae53 g_main_context_dispatch
> STDERR: 11  0x7fe260afd23e
> STDERR: 12  0x7fe260af77b1
> STDERR: 13  0x7fe260af8245
> STDERR: 14  0x7fe260af8547 ecore_main_loop_begin
> STDERR: 15  0x479cc1
> STDERR: 16  0x479e9f
> STDERR: 17  0x47a4f8 main
> STDERR: 18  0x7fe25f1a776d __libc_start_main
> STDERR: 19  0x467999

EFL layout tests are failing locally for me with the error below so I can't verify if this is fixed. But I think the error above was caused by the same mistake that is fixed in attachment 165472 [details].

crash log for DumpRenderTree (pid 12978):
STDOUT: <empty>
STDERR: ASSERTION FAILED: from.isCell() && from.asCell()->JSCell::inherits(&WTF::RemovePointer<To>::Type::s_info)
STDERR: /ssd_code/chromium/src/third_party/WebKit/Source/JavaScriptCore/runtime/JSCell.h(369) : To JSC::jsCast(JSC::JSValue) [with To = JSC::JSScope*]
STDERR: 1   0x4b2255
STDERR: 2   0x4b1396
STDERR: 3   0x4b02a4
STDERR: 4   0x4b13b0
STDERR: 5   0x4b1b2c
STDERR: 6   0x4b3d47 JSObjectMake
STDERR: 7   0x489b2d TestRunner::makeWindowObject(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue const**)
STDERR: 8   0x496a22 DumpRenderTreeChrome::onWindowObjectCleared(void*, _Evas_Object*, void*)
STDERR: 9   0x7fdc2886c483 evas_object_smart_callback_call
STDERR: 10  0x7fdc28ed2b72
STDERR: 11  0x7fdc293029e5
STDERR: 12  0x7fdc2986514e
STDERR: 13  0x7fdc28ec80cf

[Chris Dumez]

(In reply to comment #56)
> (In reply to comment #51)
> > Seems to cause assertions on EFL build bots:
> > crash log for DumpRenderTree (pid 11106):
> > STDOUT: <empty>
> > STDERR: ASSERTION FAILED: size == 0 || size >= m_encodedSize
> > STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebCore/loader/cache/CachedResource.cpp(523) : void WebCore::CachedResource::setEncodedSize(unsigned int)
> > STDERR: 1   0x7fe263acdc04 WebCore::CachedResource::setEncodedSize(unsigned int)
> > STDERR: 2   0x7fe263ac8f4f WebCore::CachedImage::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool)
> > STDERR: 3   0x7fe263a8f6ef WebCore::SubresourceLoader::sendDataToResource(char const*, int)
> > STDERR: 4   0x7fe263a8f4b0 WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, bool)
> > STDERR: 5   0x7fe263a8aa66 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
> > STDERR: 6   0x7fe264581ee2
> > STDERR: 7   0x7fe25c333765
> > STDERR: 8   0x7fe25c3488cd g_simple_async_result_complete
> > STDERR: 9   0x7fe25c348968
> > STDERR: 10  0x7fe25c67ae53 g_main_context_dispatch
> > STDERR: 11  0x7fe260afd23e
> > STDERR: 12  0x7fe260af77b1
> > STDERR: 13  0x7fe260af8245
> > STDERR: 14  0x7fe260af8547 ecore_main_loop_begin
> > STDERR: 15  0x479cc1
> > STDERR: 16  0x479e9f
> > STDERR: 17  0x47a4f8 main
> > STDERR: 18  0x7fe25f1a776d __libc_start_main
> > STDERR: 19  0x467999
> 
> EFL layout tests are failing locally for me with the error below so I can't verify if this is fixed. But I think the error above was caused by the same mistake that is fixed in attachment 165472 [details].
> 
> crash log for DumpRenderTree (pid 12978):
> STDOUT: <empty>
> STDERR: ASSERTION FAILED: from.isCell() && from.asCell()->JSCell::inherits(&WTF::RemovePointer<To>::Type::s_info)
> STDERR: /ssd_code/chromium/src/third_party/WebKit/Source/JavaScriptCore/runtime/JSCell.h(369) : To JSC::jsCast(JSC::JSValue) [with To = JSC::JSScope*]
> STDERR: 1   0x4b2255
> STDERR: 2   0x4b1396
> STDERR: 3   0x4b02a4
> STDERR: 4   0x4b13b0
> STDERR: 5   0x4b1b2c
> STDERR: 6   0x4b3d47 JSObjectMake
> STDERR: 7   0x489b2d TestRunner::makeWindowObject(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue const**)
> STDERR: 8   0x496a22 DumpRenderTreeChrome::onWindowObjectCleared(void*, _Evas_Object*, void*)
> STDERR: 9   0x7fdc2886c483 evas_object_smart_callback_call
> STDERR: 10  0x7fdc28ed2b72
> STDERR: 11  0x7fdc293029e5
> STDERR: 12  0x7fdc2986514e
> STDERR: 13  0x7fdc28ec80cf

I will test the patch on EFL port now and report back.

[Chris Dumez]

(In reply to comment #57)
> (In reply to comment #56)
> > (In reply to comment #51)
> > > Seems to cause assertions on EFL build bots:
> > > crash log for DumpRenderTree (pid 11106):
> > > STDOUT: <empty>
> > > STDERR: ASSERTION FAILED: size == 0 || size >= m_encodedSize
> > > STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebCore/loader/cache/CachedResource.cpp(523) : void WebCore::CachedResource::setEncodedSize(unsigned int)
> > > STDERR: 1   0x7fe263acdc04 WebCore::CachedResource::setEncodedSize(unsigned int)
> > > STDERR: 2   0x7fe263ac8f4f WebCore::CachedImage::data(WTF::PassRefPtr<WebCore::SharedBuffer>, bool)
> > > STDERR: 3   0x7fe263a8f6ef WebCore::SubresourceLoader::sendDataToResource(char const*, int)
> > > STDERR: 4   0x7fe263a8f4b0 WebCore::SubresourceLoader::didReceiveData(char const*, int, long long, bool)
> > > STDERR: 5   0x7fe263a8aa66 WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
> > > STDERR: 6   0x7fe264581ee2
> > > STDERR: 7   0x7fe25c333765
> > > STDERR: 8   0x7fe25c3488cd g_simple_async_result_complete
> > > STDERR: 9   0x7fe25c348968
> > > STDERR: 10  0x7fe25c67ae53 g_main_context_dispatch
> > > STDERR: 11  0x7fe260afd23e
> > > STDERR: 12  0x7fe260af77b1
> > > STDERR: 13  0x7fe260af8245
> > > STDERR: 14  0x7fe260af8547 ecore_main_loop_begin
> > > STDERR: 15  0x479cc1
> > > STDERR: 16  0x479e9f
> > > STDERR: 17  0x47a4f8 main
> > > STDERR: 18  0x7fe25f1a776d __libc_start_main
> > > STDERR: 19  0x467999
> > 
> > EFL layout tests are failing locally for me with the error below so I can't verify if this is fixed. But I think the error above was caused by the same mistake that is fixed in attachment 165472 [details] [details].
> > 
> > crash log for DumpRenderTree (pid 12978):
> > STDOUT: <empty>
> > STDERR: ASSERTION FAILED: from.isCell() && from.asCell()->JSCell::inherits(&WTF::RemovePointer<To>::Type::s_info)
> > STDERR: /ssd_code/chromium/src/third_party/WebKit/Source/JavaScriptCore/runtime/JSCell.h(369) : To JSC::jsCast(JSC::JSValue) [with To = JSC::JSScope*]
> > STDERR: 1   0x4b2255
> > STDERR: 2   0x4b1396
> > STDERR: 3   0x4b02a4
> > STDERR: 4   0x4b13b0
> > STDERR: 5   0x4b1b2c
> > STDERR: 6   0x4b3d47 JSObjectMake
> > STDERR: 7   0x489b2d TestRunner::makeWindowObject(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue const**)
> > STDERR: 8   0x496a22 DumpRenderTreeChrome::onWindowObjectCleared(void*, _Evas_Object*, void*)
> > STDERR: 9   0x7fdc2886c483 evas_object_smart_callback_call
> > STDERR: 10  0x7fdc28ed2b72
> > STDERR: 11  0x7fdc293029e5
> > STDERR: 12  0x7fdc2986514e
> > STDERR: 13  0x7fdc28ec80cf
> 
> I will test the patch on EFL port now and report back.

The latest patch does not seem to cause any crashes on EFL port. Thanks for fixing it.

[Adam Barth]

Comment on attachment 165472 [details]
Fix crash in image-empty-crash.html: was missing stillNeedsLoad check in reloadImagesIfNotDeferred. Other crashes are probably due to same mistake, but haven't checked

Ok.  Sounds like we should try landing it again.

[WebKit Review Bot]

Comment on attachment 165472 [details]
Fix crash in image-empty-crash.html: was missing stillNeedsLoad check in reloadImagesIfNotDeferred. Other crashes are probably due to same mistake, but haven't checked

Clearing flags on attachment: 165472

Committed r129462: <http://trac.webkit.org/changeset/129462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

Hi again,

fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html seems to be failing on the EFL WK1 bot: <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6332/steps/layout-test/logs/stdio>

--- /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-expected.txt
+++ /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-actual.txt
@@ -1,3 +1,3 @@
 Test that DisplayImages disabled can block http image and can be reloaded in-place when toggled.
-SUCCESS
+FAILURE: Image should not be loaded when DisplayImage is unset

[Chris Dumez]

(In reply to comment #62)
> Hi again,
> 
> fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html seems to be failing on the EFL WK1 bot: <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6332/steps/layout-test/logs/stdio>
> 
> --- /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-expected.txt
> +++ /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-actual.txt
> @@ -1,3 +1,3 @@
>  Test that DisplayImages disabled can block http image and can be reloaded in-place when toggled.
> -SUCCESS
> +FAILURE: Image should not be loaded when DisplayImage is unset

Rakuco, this is likely because the EFL port does not support overriding the "WebKitDisplayImagesKey" preference.

[Bo Liu]

(In reply to comment #63)
> (In reply to comment #62)
> > Hi again,
> > 
> > fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html seems to be failing on the EFL WK1 bot: <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6332/steps/layout-test/logs/stdio>
> > 
> > --- /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-expected.txt
> > +++ /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-actual.txt
> > @@ -1,3 +1,3 @@
> >  Test that DisplayImages disabled can block http image and can be reloaded in-place when toggled.
> > -SUCCESS
> > +FAILURE: Image should not be loaded when DisplayImage is unset
> 
> Rakuco, this is likely because the EFL port does not support overriding the "WebKitDisplayImagesKey" preference.

Sorry...just woke up. It looks like the bot is no longer failing due to this test. Was it added to a do-not-run or expect-fail list? Do I still have to worry about this?

[Chris Dumez]

(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #62)
> > > Hi again,
> > > 
> > > fast/loader/display-image-unset-can-block-image-and-can-reload-in-place.html seems to be failing on the EFL WK1 bot: <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6332/steps/layout-test/logs/stdio>
> > > 
> > > --- /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-expected.txt
> > > +++ /home/rakuco/dev/WebKit/WebKitBuild/Debug/layout-test-results/fast/loader/display-image-unset-can-block-image-and-can-reload-in-place-actual.txt
> > > @@ -1,3 +1,3 @@
> > >  Test that DisplayImages disabled can block http image and can be reloaded in-place when toggled.
> > > -SUCCESS
> > > +FAILURE: Image should not be loaded when DisplayImage is unset
> > 
> > Rakuco, this is likely because the EFL port does not support overriding the "WebKitDisplayImagesKey" preference.
> 
> Sorry...just woke up. It looks like the bot is no longer failing due to this test. Was it added to a do-not-run or expect-fail list? Do I still have to worry about this?

As I said, we don't support overriding "WebKitDisplayImagesKey" preference and the test requires it so it is not suprising if the test fails for us. We added the test to TestExpectations until we add support for this.

No need for you to worry about this :)