106162 2013-01-04 18:49:34 -0800 Pending container size requests can be cleared earlier 2013-01-07 12:35:18 -0800 1 1 1 Unclassified WebKit SVG 420+ Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 pdr pdr eric fmalita inferno japhet schenney webkit.review.bot zimmermann oldest_to_newest 800813 0 pdr 2013-01-04 18:49:34 -0800 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 801462 1 181535 pdr 2013-01-07 11:18:32 -0800 Created attachment 181535 First pass 801479 2 181535 eric 2013-01-07 11:31:07 -0800 Comment on attachment 181535 First pass Were they never being cleared? Was this effectively a leak? 801498 3 eric 2013-01-07 11:51:13 -0800 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? 801525 4 pdr 2013-01-07 12:03:14 -0800 (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. 801531 5 eric 2013-01-07 12:05:24 -0800 (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? 801536 6 pdr 2013-01-07 12:13:15 -0800 (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. 801537 7 181535 eric 2013-01-07 12:14:05 -0800 Comment on attachment 181535 First pass OK, so this really is then a perf/memory fix, and not a leak or security fix. Thanks. 801551 8 181535 webkit.review.bot 2013-01-07 12:35:14 -0800 Comment on attachment 181535 First pass Clearing flags on attachment: 181535 Committed r138976: <http://trac.webkit.org/changeset/138976> 801553 9 webkit.review.bot 2013-01-07 12:35:18 -0800 All reviewed patches have been landed. Closing bug. 181535 2013-01-07 11:18:32 -0800 2013-01-07 12:35:14 -0800 First pass 106162.1.patch text/plain 2211 pdr SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzODk2NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDEzLTAxLTA3ICBQaGlsaXAg Um9nZXJzICA8cGRyQGdvb2dsZS5jb20+CisKKyAgICAgICAgQ2xlYXIgcGVuZGluZyBjb250YWlu ZXIgc2l6ZSByZXF1ZXN0cyBhcyBlYXJseSBhcyBwb3NzaWJsZQorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA2MTYyCisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgaHR0cDovL3RyYWMud2Via2l0Lm9yZy9jaGFu Z2VzZXQvMTM3OTgxIGNvbnRhaW5lZCBhIGJ1ZyB3aGVyZSBwZW5kaW5nIGNvbnRhaW5lciBzaXpl CisgICAgICAgIHJlcXVlc3RzIHdlcmUgb25seSBjbGVhcmVkIGZvciBpbWFnZXMgdGhhdCB1c2Ug Y29udGFpbmVyIHNpemVzLiBCaXRtYXAgaW1hZ2VzIGRvIG5vdCB1c2UKKyAgICAgICAgY29udGFp bmVyIHNpemVzIHNvIHRoZXNlIHJlcXVlc3RzIHdlcmUgbm90IGJlaW5nIGNsZWFyZWQuIFRoaXMg cGF0Y2ggZG9lcyB0aGUgdHJpdmlhbAorICAgICAgICByZWZhY3RvcmluZyBzbyB0aGF0IGNvbnRh aW5lciBzaXplIHJlcXVlc3RzIGFyZSBjbGVhcmVkIGFzIGVhcmx5IGFzIHBvc3NpYmxlLgorCisg ICAgICAgIE5vIG5ldyB0ZXN0cyBhcyB0aGlzIGlzIGp1c3QgYSByZWZhY3RvcmluZyBvZiBzb21l IG1lc3N5IGNvZGUgYWRkZWQgaW4gcjEzNzk4MS4KKworICAgICAgICAqIGxvYWRlci9jYWNoZS9D YWNoZWRJbWFnZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpDYWNoZWRJbWFnZTo6Y3JlYXRlSW1h Z2UpOgorCiAyMDEzLTAxLTA3ICBNaWNoYWVsIFBydWV0dCAgPG1pY2hhZWxANjhrLm9yZz4KIAog ICAgICAgICBbSlNDXSBDb3B5IG5vbi1pbmRleCBwcm9wZXJ0aWVzIG9mIGFycmF5cyBpbiBTZXJp YWxpemVkU2NyaXB0VmFsdWUKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9DYWNo ZWRJbWFnZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvbG9hZGVyL2NhY2hlL0Nh Y2hlZEltYWdlLmNwcAkocmV2aXNpb24gMTM4ODc5KQorKysgU291cmNlL1dlYkNvcmUvbG9hZGVy L2NhY2hlL0NhY2hlZEltYWdlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzI5LDEwICszMjksMTIg QEAgaW5saW5lIHZvaWQgQ2FjaGVkSW1hZ2U6OmNyZWF0ZUltYWdlKCkKICAgICBlbHNlCiAgICAg ICAgIG1faW1hZ2UgPSBCaXRtYXBJbWFnZTo6Y3JlYXRlKHRoaXMpOwogCi0gICAgLy8gU2VuZCBx dWV1ZWQgY29udGFpbmVyIHNpemUgcmVxdWVzdHMuCi0gICAgaWYgKG1faW1hZ2UgJiYgbV9pbWFn ZS0+dXNlc0NvbnRhaW5lclNpemUoKSkgewotICAgICAgICBmb3IgKENvbnRhaW5lclNpemVSZXF1 ZXN0czo6aXRlcmF0b3IgaXQgPSBtX3BlbmRpbmdDb250YWluZXJTaXplUmVxdWVzdHMuYmVnaW4o KTsgaXQgIT0gbV9wZW5kaW5nQ29udGFpbmVyU2l6ZVJlcXVlc3RzLmVuZCgpOyArK2l0KQotICAg ICAgICAgICAgc2V0Q29udGFpbmVyU2l6ZUZvclJlbmRlcmVyKGl0LT5rZXksIGl0LT52YWx1ZS5m aXJzdCwgaXQtPnZhbHVlLnNlY29uZCk7CisgICAgaWYgKG1faW1hZ2UpIHsKKyAgICAgICAgLy8g U2VuZCBxdWV1ZWQgY29udGFpbmVyIHNpemUgcmVxdWVzdHMuCisgICAgICAgIGlmIChtX2ltYWdl LT51c2VzQ29udGFpbmVyU2l6ZSgpKSB7CisgICAgICAgICAgICBmb3IgKENvbnRhaW5lclNpemVS ZXF1ZXN0czo6aXRlcmF0b3IgaXQgPSBtX3BlbmRpbmdDb250YWluZXJTaXplUmVxdWVzdHMuYmVn aW4oKTsgaXQgIT0gbV9wZW5kaW5nQ29udGFpbmVyU2l6ZVJlcXVlc3RzLmVuZCgpOyArK2l0KQor ICAgICAgICAgICAgICAgIHNldENvbnRhaW5lclNpemVGb3JSZW5kZXJlcihpdC0+a2V5LCBpdC0+ dmFsdWUuZmlyc3QsIGl0LT52YWx1ZS5zZWNvbmQpOworICAgICAgICB9CiAgICAgICAgIG1fcGVu ZGluZ0NvbnRhaW5lclNpemVSZXF1ZXN0cy5jbGVhcigpOwogICAgIH0KIH0K