RESOLVED FIXED 106162
Pending container size requests can be cleared earlier
https://bugs.webkit.org/show_bug.cgi?id=106162
Summary Pending container size requests can be cleared earlier
Philip Rogers
Reported 2013-01-04 18:49:34 PST
CachedImage::CreateImage() only clears m_pendingContainerSizeRequests if for images that use container sizes due to a trivial mistake in http://trac.webkit.org/changeset/137981
Attachments
First pass (2.16 KB, patch)
2013-01-07 11:18 PST, Philip Rogers
no flags
Philip Rogers
Comment 1 2013-01-07 11:18:32 PST
Created attachment 181535 [details] First pass
Eric Seidel (no email)
Comment 2 2013-01-07 11:31:07 PST
Comment on attachment 181535 [details] First pass Were they never being cleared? Was this effectively a leak?
Eric Seidel (no email)
Comment 3 2013-01-07 11:51:13 PST
I guess my question is... would m_pendingContainerSizeRequests have grown w/o bounds before this? If so, it seems trivial to make a page which would trigger that. In general do we worry about this vector growing without bounds?
Philip Rogers
Comment 4 2013-01-07 12:03:14 PST
(In reply to comment #2) > (From update of attachment 181535 [details]) > Were they never being cleared? Was this effectively a leak? This isn't a leak because they would still be cleared when we destructed the CachedImage, we just keep them around too long. This issue only exists because in https://bugs.webkit.org/show_bug.cgi?id=105097#c3 you asked for me to clear these requests and I made the change without another round of reviews. This patch is just cleaning up my unreviewed mess :) (In reply to comment #3) > I guess my question is... would m_pendingContainerSizeRequests have grown w/o bounds before this? If so, it seems trivial to make a page which would trigger that. In general do we worry about this vector growing without bounds? m_pendingContainerSizeRequests should only be used between the time an image is requested and when it is created. Because these requests are stored as a pair of the renderer and the size, this vector could only grow massive if there were an infinite number of renderers all referencing a single image where all the renderers layout before the image loads.
Eric Seidel (no email)
Comment 5 2013-01-07 12:05:24 PST
(In reply to comment #4) > (In reply to comment #3) > > I guess my question is... would m_pendingContainerSizeRequests have grown w/o bounds before this? If so, it seems trivial to make a page which would trigger that. In general do we worry about this vector growing without bounds? > > m_pendingContainerSizeRequests should only be used between the time an image is requested and when it is created. Because these requests are stored as a pair of the renderer and the size, this vector could only grow massive if there were an infinite number of renderers all referencing a single image where all the renderers layout before the image loads. I see. So if you change the size of a loading-image after initial layout it does not create an additional pending size request? So an "attacker" of this vector would have to create N images in order to create N size change requests?
Philip Rogers
Comment 6 2013-01-07 12:13:15 PST
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > I guess my question is... would m_pendingContainerSizeRequests have grown w/o bounds before this? If so, it seems trivial to make a page which would trigger that. In general do we worry about this vector growing without bounds? > > > > m_pendingContainerSizeRequests should only be used between the time an image is requested and when it is created. Because these requests are stored as a pair of the renderer and the size, this vector could only grow massive if there were an infinite number of renderers all referencing a single image where all the renderers layout before the image loads. > > I see. So if you change the size of a loading-image after initial layout it does not create an additional pending size request? So an "attacker" of this vector would have to create N images in order to create N size change requests? Yeah, you've got it exactly right.
Eric Seidel (no email)
Comment 7 2013-01-07 12:14:05 PST
Comment on attachment 181535 [details] First pass OK, so this really is then a perf/memory fix, and not a leak or security fix. Thanks.
WebKit Review Bot
Comment 8 2013-01-07 12:35:14 PST
Comment on attachment 181535 [details] First pass Clearing flags on attachment: 181535 Committed r138976: <http://trac.webkit.org/changeset/138976>
WebKit Review Bot
Comment 9 2013-01-07 12:35:18 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.