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().
Created attachment 316483 [details] Patch
<rdar://problem/33530130>
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
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
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?
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.
Created attachment 316599 [details] Patch
Why is this not testable?
Created attachment 316703 [details] Test case (will crash)
Created attachment 316704 [details] Patch
Created attachment 316705 [details] Patch
Created attachment 316722 [details] Patch
Still need an answer to Jon’s question. Why no test?
Hmmwhat? There's totally a test now.
Comment on attachment 316722 [details] Patch Clearing flags on attachment: 316722 Committed r220048: <http://trac.webkit.org/changeset/220048>
All reviewed patches have been landed. Closing bug.
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>
Created attachment 316902 [details] Patch
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.
(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.
(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.
(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.
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.
I think I need to skip this new test, land this patch and file a separate bug for the memory leak bug.
Created attachment 317089 [details] Patch
(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?
(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.
Created attachment 317220 [details] Patch
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.
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 ?
Created attachment 317271 [details] Patch
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?
Comment on attachment 317271 [details] Patch Clearing flags on attachment: 317271 Committed r220289: <http://trac.webkit.org/changeset/220289>
(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.
(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.
No problem. I was not trying to prevent you from checking in; but it does seem worth some further thought and possibly further refinement.
(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.