Bug 140722 – Simplify CachedImage clients to avoid overriding both or either only imageChanged or notifyFinished

• Status: REOPENED
• Component: Images

[Julien Isorce]

Created attachment 245058 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

CachedImageClient defines imageChanged which is meant to be called whenever a frame of an image changes because we got more data from the network.

But some clients still override CachedResource::notifyFinished because in imageChanged it is not possible to know if the image is acutally enterly loaded or still loading.
(Indeed when imageChanged is called, isLoaded() never return true, even from CachedImage::finishLoading)

If we compare CachedImage to CachedFont, the later has all its clients that do not override notifyFinished. Indeed in CachedFont::finishLoading there is a call to setLoaded(false)
which is not there in CachedImage::finishLoading.

So the following patch tries to simplify the situation by marking CachedImageClient::notifyFinished final and making the refactoring that this change implies.

[Build Bot]

Comment on attachment 245058 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

Attachment 245058 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5537325064912896

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 245059 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245058 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

Attachment 245058 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4897212265922560

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 245060 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Julien Isorce]

Created attachment 245061 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

Forgot to replace ImageLoader::notifyFinished(cachedImage); by ImageLoader::imageChanged(cachedImage); in HTMLImageLoader::imageChanged.

[Tim Horton]

Comment on attachment 245061 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

This needs a changelog entry explaining the what/why.

[Julien Isorce]

Created attachment 245134 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

Add ChangeLog

[Julien Isorce]

Created attachment 245530 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

git rebase to current head

[Tim Horton]

Comment on attachment 245530 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

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

> Source/WebCore/ChangeLog:9
> +        notifyFinished was called when the image was enterely loaded.

sp.: "entirely"

> Source/WebCore/ChangeLog:13
> +        some both (ex: RenderImage).

comma at the end of this line and into the 'which'

> Source/WebCore/ChangeLog:16
> +        For example when the image finished to load, both imageChanged

"finished to load" should probably read "finished loading"

> Source/WebCore/ChangeLog:18
> +        first one isLoaded() return false.

s/return/returned/

> Source/WebCore/ChangeLog:19
> +        It could result in functions called twice in a row,

"being called"

> Source/WebCore/ChangeLog:23
> +        CachedImageClient::notifyFinished final. In order to

In order to what?

> Source/WebCore/ChangeLog:25
> +        Clients can now differenciates intermediates and end

sp.: differentiate
and intermediate should not be plural

> Source/WebCore/ChangeLog:28
> +        Because I moved setLoading(false) from CachedResource::finishLoading
> +        call to an explicit call in CachedImage::finishLoading.

Not sure what this means or if it needs to be here.

> Source/WebCore/ChangeLog:32
> +        * html/HTMLImageLoader.cpp:

It's possible you should split your changelog up down here and put relevant bits next to the names of the functions that those changes affected.

> Source/WebCore/html/HTMLImageLoader.h:38
> +    virtual void imageChanged(CachedImage*, const IntRect* = 0) override;

0 should be nullptr here.

> Source/WebCore/loader/ImageLoader.h:76
> +    virtual void imageChanged(CachedImage*, const IntRect* = 0) override;

0 should be nullptr here.

> Source/WebCore/rendering/RenderImage.cpp:-368
> -void RenderImage::notifyFinished(CachedResource* newImage)

Because of laziness, I'm not completely convinced that RenderImage::imageChanged's call to RenderImage::imageDimensionsChanged is going to always end up doing the invalidateBackgroundObscurationStatus and contentChanged that were here in all the same cases as it previously would have. Can you please confirm that it will? Most concerningly, here we unconditionally call contentChanged, but in imageDimensionsChanged we only call it conditionally (with a whole bunch of conditions).

[Julien Isorce]

Created attachment 245545 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

I addressed remarks (grammar, missing end of a sentense, file explnations, nullptr)

About the 2 other points concerning removal of RenderImage::notifyFinished:

1* invalidateBackgroundObscurationStatus:
It will not always endup with invalidateBackgroundObscurationStatus as previously.
Instead it will be done from RenderReplaced::layout() when imageDimensionsChanged calls setNeedsLayout().
The condition being: imageSizeChanged || hasOverrideSize || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize
(also RenderReplaced::layout() can be triggered this way:
Breakpoint 1, 0x00007fe784e5caf0 in WebCore::RenderReplaced::layout() ()
(gdb) bt
#0  0x00007fe784e5caf0 in WebCore::RenderReplaced::layout()
#1  0x00007fe784de92e5 in WebCore::RenderImage::layout() 
#2  0x00007fe784d8433b in WebCore::RenderBlockFlow::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) 
#3  0x00007fe784d74805 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
#4  0x00007fe784d5a056 in WebCore::RenderBlock::layout() 
#5  0x00007fe784d6f479 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) 
#6  0x00007fe784d704b8 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
#7  0x00007fe784d74d82 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
#8  0x00007fe784d5a056 in WebCore::RenderBlock::layout()
#9  0x00007fe784d6f479 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) 
#10 0x00007fe784d704b8 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) 
#11 0x00007fe784d74d82 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
#12 0x00007fe784d5a056 in WebCore::RenderBlock::layout()
#13 0x00007fe784eb62b9 in WebCore::RenderView::layoutContent(WebCore::LayoutState const&)
#14 0x00007fe784eb6719 in WebCore::RenderView::layout()
#15 0x00007fe784c20d2d in WebCore::FrameView::layout(bool)
#16 0x00007fe7848eee65 in WebCore::Document::implicitClose()
#17 0x00007fe784b57807 in WebCore::FrameLoader::checkCompleted()
#18 0x00007fe784bc09fe in WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource*, bool)
#19 0x00007fe784b80362 in WebCore::SubresourceLoader::notifyDone()
#20 0x00007fe784b812a2 in WebCore::SubresourceLoader::didFinishLoading(double)
#21 0x00007fe78507f4e8 in WebCore::readCallback(_GObject*, _GAsyncResult*, void*) 

Also it is worth to mention that the call to invalidateBackgroundObscurationStatus has been added by commit: git show cae24e092 which also added test LayoutTests/fast/backgrounds/obscured-background-child-style-change.html. And the test still passes with my patch.

2*  contentChanged(ImageChanged):
It will not always endup with contentChanged(ImageChanged) as previously.
Indeed now it is always done except if intrinsicSizeChanged && !(imageSizeChanged || hasOverrideSize || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize). Which means when it should repaint.

Also I suspect that RenderImage::notifyFinished, see commit ce3a4e007, has been added because of this setLoading(false) not being set before notifying clients (= one of the point of my patch). You can see in that commit that layer()->rendererContentChanged() (old name contentChanged(ImageChanged)) has been  put both in imageChanged and notifyFinished.
Also this commit contains test LayoutTests/compositing/direct-image-compositing.html which still passes.

[WebKit Commit Bot]

Comment on attachment 245545 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

Clearing flags on attachment: 245545

Committed r179340: <http://trac.webkit.org/changeset/179340>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 245545 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

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

> Source/WebCore/html/HTMLImageLoader.cpp:77
> +    ASSERT(cachedImage == image().get());

It broke the debug builds:

     1>..\html\HTMLImageLoader.cpp(77): error C2228: left of '.get' must have class/struct/union
                 type is 'WebCore::CachedImage *'
                 did you intend to use '->' instead?

[Julien Isorce]

Comment on attachment 245545 [details]
CachedImage: ensure clients overrides imageChanged instead of notifyFinished

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

>> Source/WebCore/html/HTMLImageLoader.cpp:77
>> +    ASSERT(cachedImage == image().get());
> 
> It broke the debug builds:
> 
>      1>..\html\HTMLImageLoader.cpp(77): error C2228: left of '.get' must have class/struct/union
>                  type is 'WebCore::CachedImage *'
>                  did you intend to use '->' instead?

Thx for raising it. It should be only image(). I am gonna make a patch right now.

[Julien Isorce]

Created attachment 245621 [details]
HTMLImageLoader: fix build failure on assert condition after r179340

[Csaba Osztrogonác]

Comment on attachment 245621 [details]
HTMLImageLoader: fix build failure on assert condition after r179340

rs=me, but please don't upload followup patches to closed bugs next time.

[Csaba Osztrogonác]

Comment on attachment 245621 [details]
HTMLImageLoader: fix build failure on assert condition after r179340

Clearing flags on attachment: 245621

Committed r179344: <http://trac.webkit.org/changeset/179344>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142598

[Andreas Kling]

This caused various problems with ImageLoaders getting stuck with m_hasPendingLoadEvent=true, which prevents the garbage collector from deleting these elements.

I'm rolling the change out since it wasn't clearly fixing a bug in the first place.

I suspect that the main cause of problems is that FrameLoader::stopAllLoaders(), which is called on navigation, ends up causing pending CachedResources to become cancelled, but cancelling a CachedResources fires notifyFinished(), not imageChanged().

[Simon Fraser (smfr)]

Could we have detected this regression in a layout test?

[Andreas Kling]

(In reply to comment #21)
> Could we have detected this regression in a layout test?

Yes, it should be possible to construct a test which schedules a bunch of image loads, and then navigates to a new page before they can complete.

The test would then have to verify that those image elements went away. It could do this using DOM node counters in window.internals, for example.

[Julien Isorce]

Hi I understand now why it has made a regression. Well there is no easy way to handle it so lets keep imageChanged/notifyFinished. Instead I suggested this patch: https://bugs.webkit.org/attachment.cgi?id=249745&action=diff. If it gets accepted and merged then I suggest to simply close the bug.