123707 2013-11-03 10:35:49 -0800 [Cocoa] Wrappers' -copyWithZone: should copy if the collection is mutable 2013-11-03 18:59:34 -0800 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mitz mitz ggaren sam oldest_to_newest 946453 0 mitz 2013-11-03 10:35:49 -0800 [Cocoa] Wrappers' -copyWithZone: should copy if the collection is mutable 946454 1 215870 mitz 2013-11-03 10:38:16 -0800 Created attachment 215870 If the collection is mutable, make a copy 946466 2 215870 ggaren 2013-11-03 12:25:33 -0800 Comment on attachment 215870 If the collection is mutable, make a copy View in context: https://bugs.webkit.org/attachment.cgi?id=215870&action=review > Source/WebKit2/Shared/ImmutableArray.h:68 > + const Vector<RefPtr<APIObject>> entries() { return m_entries; } Should this be Vector& instead of Vector? As written, each call to entries() will make a copy. > Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63 > + auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries(); If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake). 946467 3 215870 mitz 2013-11-03 12:28:32 -0800 Comment on attachment 215870 If the collection is mutable, make a copy View in context: https://bugs.webkit.org/attachment.cgi?id=215870&action=review >> Source/WebKit2/Shared/ImmutableArray.h:68 >> + const Vector<RefPtr<APIObject>> entries() { return m_entries; } > > Should this be Vector& instead of Vector? As written, each call to entries() will make a copy. Yes. >> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63 >> + auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries(); > > If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake). This needs to be a copy, not a reference. 946468 4 215872 mitz 2013-11-03 12:30:49 -0800 Created attachment 215872 If the collection is mutable, make a copy 946472 5 215872 ggaren 2013-11-03 12:37:03 -0800 Comment on attachment 215872 If the collection is mutable, make a copy r=me 946473 6 ggaren 2013-11-03 12:38:55 -0800 > >> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63 > >> + auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries(); > > > > If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake). > > This needs to be a copy, not a reference. What I meant was that, if you had used const ImmutableArray::VectorType& entries = ... you would have gotten an error, due to taking a reference to a temporary, which might have alerted you to the accidental copy caused by the return type of entries(). I think about stuff like this because I'm still not sure when we should use auto, and when we should not. 946474 7 mitz 2013-11-03 12:41:13 -0800 (In reply to comment #6) > > >> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:63 > > >> + auto entries = reinterpret_cast<ImmutableArray*>(&_array)->entries(); > > > > > > If you had not used auto in this context, you would have use an explicit reference type, in which case the compiler would have caught the extra copy mistake (assuming I am right that it was a mistake). > > > > This needs to be a copy, not a reference. > > What I meant was that, if you had used > > const ImmutableArray::VectorType& entries = ... > > you would have gotten an error, due to taking a reference to a temporary, which might have alerted you to the accidental copy caused by the return type of entries(). But, regardless of my mistake in the definition of entries(), I wouldn’t have used the above type for the local, since I did want a copy, not a reference. 946475 8 mitz 2013-11-03 12:41:27 -0800 Committed <http://trac.webkit.org/r158538>. 946477 9 ggaren 2013-11-03 12:48:42 -0800 > But, regardless of my mistake in the definition of entries(), I wouldn’t have used the above type for the local, since I did want a copy, not a reference. Uh oh. Since the patch you committed passes a reference from _array (and _dictionary) to adopt(), and adopt() calls swap() -- does this mean that -copyWithZone will remove all entries from self? I am demonstrating my limitations as a reviewer :(. 946479 10 mitz 2013-11-03 12:56:25 -0800 (In reply to comment #9) > > But, regardless of my mistake in the definition of entries(), I wouldn’t have used the above type for the local, since I did want a copy, not a reference. > > Uh oh. Since the patch you committed passes a reference from _array (and _dictionary) to adopt(), and adopt() calls swap() -- does this mean that -copyWithZone will remove all entries from self? The locals’ types aren’t const references. They are a Vector and a MapType. 946526 11 mitz 2013-11-03 18:57:26 -0800 (In reply to comment #8) > Committed <http://trac.webkit.org/r158538>. I shouldn’t have landed this patch without a WebKit2 owner’s permission. I reverted the change in <http://trac.webkit.org/r158552>. 946527 12 mitz 2013-11-03 18:59:34 -0800 …and committed <http://trac.webkit.org/r158553>. 215870 2013-11-03 10:38:16 -0800 2013-11-03 12:30:47 -0800 If the collection is mutable, make a copy bug-123707-20131103103820.patch text/plain 2710 mitz SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1ODUzNSkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEzLTExLTAzICBEYW4gQmVy bnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CisKKyAgICAgICAgW0NvY29hXSBXcmFwcGVycycgLWNv cHlXaXRoWm9uZTogc2hvdWxkIGNvcHkgaWYgdGhlIGNvbGxlY3Rpb24gaXMgbXV0YWJsZQorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIzNzA3CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBTaGFyZWQvQ29j b2EvV0tOU0FycmF5Lm1tOgorICAgICAgICAoLVtXS05TQXJyYXkgY29weVdpdGhab25lOl0pOiBJ ZiB0aGUgYXJyYXkgaXMgbXV0YWJsZSwgbWFrZSBhIGNvcHkuCisgICAgICAgICogU2hhcmVkL0Nv Y29hL1dLTlNEaWN0aW9uYXJ5Lm1tOgorICAgICAgICAoLVtXS05TRGljdGlvbmFyeSBjb3B5V2l0 aFpvbmU6XSk6IElmIHRoZSBkaWN0aW9uYXJ5IGlzIG11dGFibGUsIG1ha2UgYSBjb3B5LgorICAg ICAgICAqIFNoYXJlZC9JbW11dGFibGVBcnJheS5oOgorICAgICAgICAoV2ViS2l0OjpJbW11dGFi bGVBcnJheTo6ZW50cmllcyk6IEFkZGVkIHRoaXMgYWNjZXNzb3IuCisKIDIwMTMtMTEtMDIgIERh biBCZXJuc3RlaW4gIDxtaXR6QGFwcGxlLmNvbT4KIAogICAgICAgICBbQ29jb2FdIFdyYXBwZXJz IG1pc2hhbmRsZSBOVUxMIHZhbHVlcyBpbiBhcnJheXMgYW5kIGRpY3Rpb25hcmllcwpJbmRleDog U291cmNlL1dlYktpdDIvU2hhcmVkL0ltbXV0YWJsZUFycmF5LmgKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYktpdDIvU2hhcmVkL0ltbXV0YWJsZUFycmF5LmgJKHJldmlzaW9uIDE1ODQ5MykKKysr IFNvdXJjZS9XZWJLaXQyL1NoYXJlZC9JbW11dGFibGVBcnJheS5oCSh3b3JraW5nIGNvcHkpCkBA IC02NSw2ICs2NSw4IEBAIHB1YmxpYzoKIAogICAgIHZpcnR1YWwgYm9vbCBpc011dGFibGUoKSB7 IHJldHVybiBmYWxzZTsgfQogCisgICAgY29uc3QgVmVjdG9yPFJlZlB0cjxBUElPYmplY3Q+PiBl bnRyaWVzKCkgeyByZXR1cm4gbV9lbnRyaWVzOyB9CisKIHByb3RlY3RlZDoKICAgICBJbW11dGFi bGVBcnJheSgpOwogICAgIEltbXV0YWJsZUFycmF5KEFkb3B0VGFnLCBBUElPYmplY3QqKiBlbnRy aWVzLCBzaXplX3QpOwpJbmRleDogU291cmNlL1dlYktpdDIvU2hhcmVkL0NvY29hL1dLTlNBcnJh eS5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9TaGFyZWQvQ29jb2EvV0tOU0FycmF5 Lm1tCShyZXZpc2lvbiAxNTg0OTMpCisrKyBTb3VyY2UvV2ViS2l0Mi9TaGFyZWQvQ29jb2EvV0tO U0FycmF5Lm1tCSh3b3JraW5nIGNvcHkpCkBAIC01Nyw3ICs1NywxMSBAQCAtIChpZClvYmplY3RB dEluZGV4OihOU1VJbnRlZ2VyKWkKIAogLSAoaWQpY29weVdpdGhab25lOihOU1pvbmUgKil6b25l CiB7Ci0gICAgcmV0dXJuIFtzZWxmIHJldGFpbl07CisgICAgaWYgKCFyZWludGVycHJldF9jYXN0 PEltbXV0YWJsZUFycmF5Kj4oJl9hcnJheSktPmlzTXV0YWJsZSgpKQorICAgICAgICByZXR1cm4g W3NlbGYgcmV0YWluXTsKKworICAgIGF1dG8gZW50cmllcyA9IHJlaW50ZXJwcmV0X2Nhc3Q8SW1t dXRhYmxlQXJyYXkqPigmX2FycmF5KS0+ZW50cmllcygpOworICAgIHJldHVybiBJbW11dGFibGVB cnJheTo6YWRvcHQoZW50cmllcykubGVha1JlZigpLT53cmFwcGVyKCk7CiB9CiAKICNwcmFnbWEg bWFyayBXS09iamVjdCBwcm90b2NvbCBpbXBsZW1lbnRhdGlvbgpJbmRleDogU291cmNlL1dlYktp dDIvU2hhcmVkL0NvY29hL1dLTlNEaWN0aW9uYXJ5Lm1tCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJLaXQyL1NoYXJlZC9Db2NvYS9XS05TRGljdGlvbmFyeS5tbQkocmV2aXNpb24gMTU4NDkzKQor KysgU291cmNlL1dlYktpdDIvU2hhcmVkL0NvY29hL1dLTlNEaWN0aW9uYXJ5Lm1tCSh3b3JraW5n IGNvcHkpCkBAIC03Niw3ICs3NiwxMSBAQCAtIChOU0VudW1lcmF0b3IgKilrZXlFbnVtZXJhdG9y CiAKIC0gKGlkKWNvcHlXaXRoWm9uZTooTlNab25lICopem9uZQogewotICAgIHJldHVybiBbc2Vs ZiByZXRhaW5dOworICAgIGlmICghcmVpbnRlcnByZXRfY2FzdDxJbW11dGFibGVEaWN0aW9uYXJ5 Kj4oJl9kaWN0aW9uYXJ5KS0+aXNNdXRhYmxlKCkpCisgICAgICAgIHJldHVybiBbc2VsZiByZXRh aW5dOworCisgICAgYXV0byBtYXAgPSByZWludGVycHJldF9jYXN0PEltbXV0YWJsZURpY3Rpb25h cnkqPigmX2RpY3Rpb25hcnkpLT5tYXAoKTsKKyAgICByZXR1cm4gSW1tdXRhYmxlRGljdGlvbmFy eTo6YWRvcHQobWFwKS5sZWFrUmVmKCktPndyYXBwZXIoKTsKIH0KIAogI3ByYWdtYSBtYXJrIFdL T2JqZWN0IHByb3RvY29sIGltcGxlbWVudGF0aW9uCg== 215872 2013-11-03 12:30:49 -0800 2013-11-03 12:37:03 -0800 If the collection is mutable, make a copy bug-123707-20131103123055.patch text/plain 2711 mitz SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1ODUzNSkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEzLTExLTAzICBEYW4gQmVy bnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CisKKyAgICAgICAgW0NvY29hXSBXcmFwcGVycycgLWNv cHlXaXRoWm9uZTogc2hvdWxkIGNvcHkgaWYgdGhlIGNvbGxlY3Rpb24gaXMgbXV0YWJsZQorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTIzNzA3CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBTaGFyZWQvQ29j b2EvV0tOU0FycmF5Lm1tOgorICAgICAgICAoLVtXS05TQXJyYXkgY29weVdpdGhab25lOl0pOiBJ ZiB0aGUgYXJyYXkgaXMgbXV0YWJsZSwgbWFrZSBhIGNvcHkuCisgICAgICAgICogU2hhcmVkL0Nv Y29hL1dLTlNEaWN0aW9uYXJ5Lm1tOgorICAgICAgICAoLVtXS05TRGljdGlvbmFyeSBjb3B5V2l0 aFpvbmU6XSk6IElmIHRoZSBkaWN0aW9uYXJ5IGlzIG11dGFibGUsIG1ha2UgYSBjb3B5LgorICAg ICAgICAqIFNoYXJlZC9JbW11dGFibGVBcnJheS5oOgorICAgICAgICAoV2ViS2l0OjpJbW11dGFi bGVBcnJheTo6ZW50cmllcyk6IEFkZGVkIHRoaXMgYWNjZXNzb3IuCisKIDIwMTMtMTEtMDIgIERh biBCZXJuc3RlaW4gIDxtaXR6QGFwcGxlLmNvbT4KIAogICAgICAgICBbQ29jb2FdIFdyYXBwZXJz IG1pc2hhbmRsZSBOVUxMIHZhbHVlcyBpbiBhcnJheXMgYW5kIGRpY3Rpb25hcmllcwpJbmRleDog U291cmNlL1dlYktpdDIvU2hhcmVkL0ltbXV0YWJsZUFycmF5LmgKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYktpdDIvU2hhcmVkL0ltbXV0YWJsZUFycmF5LmgJKHJldmlzaW9uIDE1ODQ5MykKKysr IFNvdXJjZS9XZWJLaXQyL1NoYXJlZC9JbW11dGFibGVBcnJheS5oCSh3b3JraW5nIGNvcHkpCkBA IC02NSw2ICs2NSw4IEBAIHB1YmxpYzoKIAogICAgIHZpcnR1YWwgYm9vbCBpc011dGFibGUoKSB7 IHJldHVybiBmYWxzZTsgfQogCisgICAgY29uc3QgVmVjdG9yPFJlZlB0cjxBUElPYmplY3Q+PiYg ZW50cmllcygpIHsgcmV0dXJuIG1fZW50cmllczsgfQorCiBwcm90ZWN0ZWQ6CiAgICAgSW1tdXRh YmxlQXJyYXkoKTsKICAgICBJbW11dGFibGVBcnJheShBZG9wdFRhZywgQVBJT2JqZWN0KiogZW50 cmllcywgc2l6ZV90KTsKSW5kZXg6IFNvdXJjZS9XZWJLaXQyL1NoYXJlZC9Db2NvYS9XS05TQXJy YXkubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvU2hhcmVkL0NvY29hL1dLTlNBcnJh eS5tbQkocmV2aXNpb24gMTU4NDkzKQorKysgU291cmNlL1dlYktpdDIvU2hhcmVkL0NvY29hL1dL TlNBcnJheS5tbQkod29ya2luZyBjb3B5KQpAQCAtNTcsNyArNTcsMTEgQEAgLSAoaWQpb2JqZWN0 QXRJbmRleDooTlNVSW50ZWdlcilpCiAKIC0gKGlkKWNvcHlXaXRoWm9uZTooTlNab25lICopem9u ZQogewotICAgIHJldHVybiBbc2VsZiByZXRhaW5dOworICAgIGlmICghcmVpbnRlcnByZXRfY2Fz dDxJbW11dGFibGVBcnJheSo+KCZfYXJyYXkpLT5pc011dGFibGUoKSkKKyAgICAgICAgcmV0dXJu IFtzZWxmIHJldGFpbl07CisKKyAgICBhdXRvIGVudHJpZXMgPSByZWludGVycHJldF9jYXN0PElt bXV0YWJsZUFycmF5Kj4oJl9hcnJheSktPmVudHJpZXMoKTsKKyAgICByZXR1cm4gSW1tdXRhYmxl QXJyYXk6OmFkb3B0KGVudHJpZXMpLmxlYWtSZWYoKS0+d3JhcHBlcigpOwogfQogCiAjcHJhZ21h IG1hcmsgV0tPYmplY3QgcHJvdG9jb2wgaW1wbGVtZW50YXRpb24KSW5kZXg6IFNvdXJjZS9XZWJL aXQyL1NoYXJlZC9Db2NvYS9XS05TRGljdGlvbmFyeS5tbQo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V2ViS2l0Mi9TaGFyZWQvQ29jb2EvV0tOU0RpY3Rpb25hcnkubW0JKHJldmlzaW9uIDE1ODQ5MykK KysrIFNvdXJjZS9XZWJLaXQyL1NoYXJlZC9Db2NvYS9XS05TRGljdGlvbmFyeS5tbQkod29ya2lu ZyBjb3B5KQpAQCAtNzYsNyArNzYsMTEgQEAgLSAoTlNFbnVtZXJhdG9yICopa2V5RW51bWVyYXRv cgogCiAtIChpZCljb3B5V2l0aFpvbmU6KE5TWm9uZSAqKXpvbmUKIHsKLSAgICByZXR1cm4gW3Nl bGYgcmV0YWluXTsKKyAgICBpZiAoIXJlaW50ZXJwcmV0X2Nhc3Q8SW1tdXRhYmxlRGljdGlvbmFy eSo+KCZfZGljdGlvbmFyeSktPmlzTXV0YWJsZSgpKQorICAgICAgICByZXR1cm4gW3NlbGYgcmV0 YWluXTsKKworICAgIGF1dG8gbWFwID0gcmVpbnRlcnByZXRfY2FzdDxJbW11dGFibGVEaWN0aW9u YXJ5Kj4oJl9kaWN0aW9uYXJ5KS0+bWFwKCk7CisgICAgcmV0dXJuIEltbXV0YWJsZURpY3Rpb25h cnk6OmFkb3B0KG1hcCkubGVha1JlZigpLT53cmFwcGVyKCk7CiB9CiAKICNwcmFnbWEgbWFyayBX S09iamVjdCBwcm90b2NvbCBpbXBsZW1lbnRhdGlvbgo=