Bug 174874 – RenderImageResourceStyleImage::image() should return the nullImage() if the image is not available

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

Like the base class virtual method RenderImageResource::image(), RenderImageResourceStyleImage::image() should return the nullImage() if the image is not available.

This is to prevent a crash we saw recently because of calling image()->stopAnimation() in RenderImageResourceStyleImage::shutdown(). The crash was happening because RenderImageResourceStyleImage::image() can return a null Image pointer . m_styleImage->image() can return a null pointer via StyleCachedImage::image() or StyleGeneratedImage::image().

[Said Abou-Hallawa]

Created attachment 316483 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/33530130>

[Build Bot]

Comment on attachment 316483 [details]
Patch

Attachment 316483 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4192746

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 316494 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Jon Lee]

Comment on attachment 316483 [details]
Patch

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

Is this testable?

> Source/WebCore/ChangeLog:5
> +

Please add Radar.

> Source/WebCore/ChangeLog:14
> +        of r208511 in this function. Add a call to image()->stopAnimation() without

you mean 219205?

[Said Abou-Hallawa]

Comment on attachment 316483 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        of r208511 in this function. Add a call to image()->stopAnimation() without
> 
> you mean 219205?

No. I meant r208511 because I am talking about RenderImageResourceStyleImage::shutdown() which was like this before r208511:

void RenderImageResourceStyleImage::shutdown()
{
    ASSERT(m_renderer);
    m_styleImage->removeClient(m_renderer);
    m_cachedImage = nullptr;
}

And with this patch it is going to look like this:

void RenderImageResourceStyleImage::shutdown()
{
    ASSERT(m_renderer);
    image()->stopAnimation();
    m_styleImage->removeClient(m_renderer);
    m_cachedImage = nullptr;
}

But in r219205, I changed RenderImageResourceStyleImage::image() from 

RefPtr<Image> RenderImageResourceStyleImage::image(const IntSize& size) const
{
    // Generated content may trigger calls to image() while we're still pending, don't assert but gracefully exit.
    if (m_styleImage->isPending())
        return nullptr;
    return m_styleImage->image(m_renderer, size);
}

to be

RefPtr<Image> RenderImageResourceStyleImage::image(const IntSize& size) const
{
    // Generated content may trigger calls to image() while we're still pending, don't assert but gracefully exit.
    return !m_styleImage->isPending() ? m_styleImage->image(m_renderer, size) : &Image::nullImage();
}

I was trying to get it to be similar to the base class virtual function RenderImageResource::image() which returns the nullImage() if the image not available. But I did not do quite right because m_styleImage->image() can still return a null pointer. So in this patch I am trying to ensure RenderImageResourceStyleImage::image() returns either a valid Image pointer or the nullImage() if the image is not available.

[Said Abou-Hallawa]

Created attachment 316599 [details]
Patch

[Jon Lee]

Why is this not testable?

[Said Abou-Hallawa]

Created attachment 316703 [details]
Test case (will crash)

[Said Abou-Hallawa]

Created attachment 316704 [details]
Patch

[Said Abou-Hallawa]

Created attachment 316705 [details]
Patch

[Said Abou-Hallawa]

Created attachment 316722 [details]
Patch

[Darin Adler]

Still need an answer to Jon’s question. Why no test?

[Tim Horton]

Hmmwhat? There's totally a test now.

[WebKit Commit Bot]

Comment on attachment 316722 [details]
Patch

Clearing flags on attachment: 316722

Committed r220048: <http://trac.webkit.org/changeset/220048>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

Reverted r220048 for reason:

This revision caused multiple crashes in fast/images. See webkit.org/b/174990

Committed r220073: <http://trac.webkit.org/changeset/220073>

[Said Abou-Hallawa]

Created attachment 316902 [details]
Patch

[Said Abou-Hallawa]

The changed functions:

RenderImageResourceStyleImage::shutdown()
RenderImageResourceStyleImage::image()

do not even get called when running the crashed tests

fast/images/low-memory-decode.html
fast/images/link-body-content-imageDimensionChanged-crash.html

The only thing that I can think of that may have caused these crashes is removing including the style image header files from some sources. So I reverted these changes and I uploaded a new patch with the changes in the above two functions only.

[Jonathan Bedard]

(In reply to Said Abou-Hallawa from comment #19)
> The changed functions:
> 
> RenderImageResourceStyleImage::shutdown()
> RenderImageResourceStyleImage::image()
> 
> do not even get called when running the crashed tests
> 
> fast/images/low-memory-decode.html
> fast/images/link-body-content-imageDimensionChanged-crash.html
> 
> The only thing that I can think of that may have caused these crashes is
> removing including the style image header files from some sources. So I
> reverted these changes and I uploaded a new patch with the changes in the
> above two functions only.

I'm thinking that you've triggered some stateful flakiness in the test runner.  I'm going to look into this tomorrow morning.

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #20)
> ...
> 
> I'm thinking that you've triggered some stateful flakiness in the test
> runner.  I'm going to look into this tomorrow morning.

Ok, this is actually pretty easy to reproduce.

If you run 'run-webkit-tests --child-processes=1 fast/images' with the patch vs the same command without the patch, you will see a leak in fast/images/low-memory-decode.html which we treat as a crash.

Not sure how your patch is causing this, but I'd like to see that resolved before this lands again.

[Said Abou-Hallawa]

(In reply to Jonathan Bedard from comment #21)
> (In reply to Jonathan Bedard from comment #20)
> > ...
> > 
> > I'm thinking that you've triggered some stateful flakiness in the test
> > runner.  I'm going to look into this tomorrow morning.
> 
> Ok, this is actually pretty easy to reproduce.
> 
> If you run 'run-webkit-tests --child-processes=1 fast/images' with the patch
> vs the same command without the patch, you will see a leak in
> fast/images/low-memory-decode.html which we treat as a crash.
> 
> Not sure how your patch is causing this, but I'd like to see that resolved
> before this lands again.

I ran 'run-webkit-tests --child-processes=1 fast/images' with the patch but without the new test, which I added in this patch, LayoutTests/fast/images/image-element-image-content-data.html, and I did not see a leak in fast/images/low-memory-decode.html.

[Said Abou-Hallawa]

Created attachment 317085 [details]
leak_results

I ran 'run-webkit-tests --child-processes=1  --leak LayoutTests/fast/images/image-element-image-content-data.html --repeat-each=10 -1' and I got the attached leak_results. I could not find in this file anything related to the CachedImage or the RenderImageResourceStyleImage.

[Said Abou-Hallawa]

I think I need to skip this new test, land this patch and file a separate bug for the memory leak bug.

[Said Abou-Hallawa]

Created attachment 317089 [details]
Patch

[Jonathan Bedard]

(In reply to Said Abou-Hallawa from comment #24)
> I think I need to skip this new test, land this patch and file a separate
> bug for the memory leak bug.

What happens if you run the tests with the new test but without the patch?  Do you still see a leak?

[Said Abou-Hallawa]

(In reply to Jonathan Bedard from comment #26)
> (In reply to Said Abou-Hallawa from comment #24)
> > I think I need to skip this new test, land this patch and file a separate
> > bug for the memory leak bug.
> 
> What happens if you run the tests with the new test but without the patch? 
> Do you still see a leak?

When running the test without the C++ changes (expect a null check to prevent the crash), I was getting a crash in fast/images/low-memory-decode.html and a memory leak in LayoutTests/fast/images/image-element-image-content-data.html.

After <http://trac.webkit.org/changeset/220183>, the memory leak is gone but still the crash still exists. This crash is different from the crash we see when opening the attached test case. It happens in CachedImage::canDestroyDecodedData() when accessing the local variable 'client'. The call stack is similar to this: https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r220068%20(2283)/fast/images/low-memory-decode-crash-log.txt

To confirm this is the case, I changed CachedImage::canDestroyDecodedData() such that it returns false always without accessing any of the CacheImageClients. I ran

run-webkit-tests --debug --child-processes=1 LayoutTests/fast/images/
run-webkit-tests --child-processes=1 --leak LayoutTests/fast/images/image-element-image-content-data.html --repeat-each=100 -1

And all the tests including the new test passed without crashes or memory leaks.

[Said Abou-Hallawa]

Created attachment 317220 [details]
Patch

[Said Abou-Hallawa]

I figured out why the other crash was happening. It can happen if a RenderImageResourceStyleImage has its m_styleImage of type StyleGeneratedImage but its m_cachedImage is set when loading the src url finishes. RenderImageResourceStyleImage::shutdown() does not remove m_renderer from the clients of m_cachedImage. So we end having a freed pointer in the CachedImage::m_clients. If the CachedImage is deleted by the MemoryCache, it is going to call BitmapImage::destroyDecodedData() which will end up calling CachedImage::canDestroyDecodedData(). This function function will enumerate the clients stored in m_clients. The crash will happen when trying to call canDestroyDecodedData() of the freed RenderImage client.

[Simon Fraser (smfr)]

Comment on attachment 317220 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        If an <img> element has a none CachedImage content data, e.g.

Do you mean non-CachedImage ?

[Said Abou-Hallawa]

Created attachment 317271 [details]
Patch

[Darin Adler]

Comment on attachment 317271 [details]
Patch

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

> Source/WebCore/rendering/RenderImageResourceStyleImage.cpp:62
> +    if (!m_styleImage->isCachedImage() && m_cachedImage)
> +        m_cachedImage->removeClient(*m_renderer);

This code is really confusing and does not seem right. I would expect the removeClient call would be made by the same class as the addClient call, and be a counterpart to that call. Where is the addClient call that this removeClient call balances?

[WebKit Commit Bot]

Comment on attachment 317271 [details]
Patch

Clearing flags on attachment: 317271

Committed r220289: <http://trac.webkit.org/changeset/220289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to Darin Adler from comment #32)
> Comment on attachment 317271 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317271&action=review
> 
> > Source/WebCore/rendering/RenderImageResourceStyleImage.cpp:62
> > +    if (!m_styleImage->isCachedImage() && m_cachedImage)
> > +        m_cachedImage->removeClient(*m_renderer);
> 
> This code is really confusing and does not seem right. I would expect the
> removeClient call would be made by the same class as the addClient call, and
> be a counterpart to that call. Where is the addClient call that this
> removeClient call balances?

At some point I would like to hear the answer to this code style/structure question. I think some kind of change here can improve clarity.

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #35)
> (In reply to Darin Adler from comment #32)
> > Comment on attachment 317271 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=317271&action=review
> > 
> > > Source/WebCore/rendering/RenderImageResourceStyleImage.cpp:62
> > > +    if (!m_styleImage->isCachedImage() && m_cachedImage)
> > > +        m_cachedImage->removeClient(*m_renderer);
> > 
> > This code is really confusing and does not seem right. I would expect the
> > removeClient call would be made by the same class as the addClient call, and
> > be a counterpart to that call. Where is the addClient call that this
> > removeClient call balances?
> 
> At some point I would like to hear the answer to this code style/structure
> question. I think some kind of change here can improve clarity.

I am sorry for missing your comment. When I received the bugzilla mail saying the patch is +r'ed by smfr, I just +cq'ed it without refreshing the page. I will address your comment in a follow up patch.

[Darin Adler]

No problem. I was not trying to prevent you from checking in; but it does seem worth some further thought and possibly further refinement.

[Said Abou-Hallawa]

(In reply to Darin Adler from comment #37)
> No problem. I was not trying to prevent you from checking in; but it does
> seem worth some further thought and possibly further refinement.

Followup patch is in https://bugs.webkit.org/show_bug.cgi?id=175444.