Summary: | Image should clear its ImageObserver* when CachedImage releases the last reference to its RefCounted<ImageObserver> | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Component: | Images | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, darin, dbates, japhet, rniwa, ryanhaddad, simon.fraser, thorton, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2017-06-07 15:07:17 PDT
Created attachment 312239 [details]
Patch
Comment on attachment 312239 [details] Patch Attachment 312239 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3890363 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.html Created attachment 312249 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 312315 [details]
Patch
Comment on attachment 312315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312315&action=review > Source/WebCore/loader/cache/CachedImage.cpp:375 > + if (!m_imageObserver->cachedImages().size()) { isEmpty() Is this testable? Created attachment 312450 [details]
Patch
Comment on attachment 312450 [details] Patch Clearing flags on attachment: 312450 Committed r218003: <http://trac.webkit.org/changeset/218003> All reviewed patches have been landed. Closing bug. (In reply to WebKit Commit Bot from comment #9) > Comment on attachment 312450 [details] > Patch > > Clearing flags on attachment: 312450 > > Committed r218003: <http://trac.webkit.org/changeset/218003> I'm seeing a few LayoutTests hit one of the asserts added with this change: http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-canvas-element/size.attributes.removed.html https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r218012%20(1627)/results.html Reverted r218003 for reason: This change caused assertion failures in existing LayoutTests. Committed r218031: <http://trac.webkit.org/changeset/218031> (In reply to Ryan Haddad from comment #11) > (In reply to WebKit Commit Bot from comment #9) > > Comment on attachment 312450 [details] > > Patch > > > > Clearing flags on attachment: 312450 > > > > Committed r218003: <http://trac.webkit.org/changeset/218003> > > I'm seeing a few LayoutTests hit one of the asserts added with this change: > > http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials. > html > imported/w3c/web-platform-tests/html/semantics/embedded-content/the-canvas- > element/size.attributes.removed.html > > https://build.webkit.org/results/ > Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r218012%20(1627)/results.html Yes. There is no guarantee that the Image is referenced only by CachedImage. One of the CachedImage constructors takes an argument of Image* which means the Image object is created and referenced outside the life cycle of CachedImage. When storing it in CachedImage::m_image, the refcount is incremented. So the Image object can outlive CachedImage. The only thing we can guarantee, is CachedImageObserver can’t outlive CachedImage. So the fix is to remove ASSERT(m_image->hasOneRef()) from CachedImage::clearImage(). Created attachment 312530 [details]
Patch
Comment on attachment 312530 [details] Patch Clearing flags on attachment: 312530 Committed r218038: <http://trac.webkit.org/changeset/218038> All reviewed patches have been landed. Closing bug. |