Bug 106162 – Pending container size requests can be cleared earlier

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

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

[Philip Rogers]

Created attachment 181535 [details]
First pass

[Eric Seidel (no email)]

Comment on attachment 181535 [details]
First pass

Were they never being cleared?  Was this effectively a leak?

[Eric Seidel (no email)]

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]

(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)]

(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]

(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 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 on attachment 181535 [details]
First pass

Clearing flags on attachment: 181535

Committed r138976: <http://trac.webkit.org/changeset/138976>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.