97955 2012-09-28 17:31:55 -0700 Fix compilation of V8DependentRetained and JSDependentRetained. 2012-09-28 19:16:07 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 93661 1 esprehn esprehn abarth haraken japhet webkit.review.bot oldest_to_newest 731088 0 esprehn 2012-09-28 17:31:55 -0700 Fix compilation of V8DependentRetained and JSDependentRetained. 731089 1 166337 esprehn 2012-09-28 17:34:01 -0700 Created attachment 166337 Patch 731101 2 166337 haraken 2012-09-28 17:52:08 -0700 Comment on attachment 166337 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review > Source/WebCore/ChangeLog:8 > + Fix bad usage of putDirect and removeDirect from JSDependentRetained and fix The JSC change looks good. > Source/WebCore/ChangeLog:9 > + incorrect assumptions about how weak handles work in V8. This is refactored The point of the V8 fix is: - Use m_owner.set() instead of m_owern =. - Call value->release() in the weak callback. Am I understanding correctly? > Source/WebCore/bindings/v8/V8DependentRetained.h:37 > +#include <wtf/text/StringBuilder.h> Nit: Why is this needed? 731103 3 166337 esprehn 2012-09-28 17:57:16 -0700 Comment on attachment 166337 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review >> Source/WebCore/ChangeLog:9 >> + incorrect assumptions about how weak handles work in V8. This is refactored > > The point of the V8 fix is: > > - Use m_owner.set() instead of m_owern =. > - Call value->release() in the weak callback. > > Am I understanding correctly? - Yeah, need to use set() since ScopedPersistent doesn't have overloaded operator=. - I originally had one weakCallback and abarth suggested using two callbacks, one for the owner and one for the value and asserting about the order, but that was a wrong thing to do since the order of the callbacks doesn't seem to work as expected when v8 destroys everything (whole page navigate). No code uses this code yet, but if you did you'd get asserts. >> Source/WebCore/bindings/v8/V8DependentRetained.h:37 >> +#include <wtf/text/StringBuilder.h> > > Nit: Why is this needed? The code didn't compile because someone else refactored it before my other patch landed that actually uses this, and they made it use StringBuilder (see in the create createPropertyName method in the file). No one noticed it broke because nothing uses this yet. 731104 4 166337 haraken 2012-09-28 17:59:07 -0700 Comment on attachment 166337 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review >>> Source/WebCore/ChangeLog:9 >>> + incorrect assumptions about how weak handles work in V8. This is refactored >> >> The point of the V8 fix is: >> >> - Use m_owner.set() instead of m_owern =. >> - Call value->release() in the weak callback. >> >> Am I understanding correctly? > > - Yeah, need to use set() since ScopedPersistent doesn't have overloaded operator=. > - I originally had one weakCallback and abarth suggested using two callbacks, one for the owner and one for the value and asserting about the order, but that was a wrong thing to do since the order of the callbacks doesn't seem to work as expected when v8 destroys everything (whole page navigate). No code uses this code yet, but if you did you'd get asserts. Makes sense. Thanks for the clarification! 731135 5 166337 webkit.review.bot 2012-09-28 19:16:04 -0700 Comment on attachment 166337 Patch Clearing flags on attachment: 166337 Committed r129968: <http://trac.webkit.org/changeset/129968> 731136 6 webkit.review.bot 2012-09-28 19:16:07 -0700 All reviewed patches have been landed. Closing bug. 166337 2012-09-28 17:34:01 -0700 2012-09-28 19:16:04 -0700 Patch bug-97955-20120928173318.patch text/plain 5119 esprehn U3VidmVyc2lvbiBSZXZpc2lvbjogMTI5OTYxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2ViMTcyN2VkODNjNDA0 MmQ4YzQ0YTg0YTJhYTk1OGE1YjNkYzFhNi4uOTliZTY3N2JjNTJiOGQ1NzAwZDZlY2QyZmQ4NTgw MGYxY2I1YTVjNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2IEBACisyMDEyLTA5LTI4ICBFbGxp b3R0IFNwcmVobiAgPGVzcHJlaG5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIEZpeCBjb21waWxh dGlvbiBvZiBWOERlcGVuZGVudFJldGFpbmVkIGFuZCBKU0RlcGVuZGVudFJldGFpbmVkLgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTc5NTUKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggYmFkIHVzYWdl IG9mIHB1dERpcmVjdCBhbmQgcmVtb3ZlRGlyZWN0IGZyb20gSlNEZXBlbmRlbnRSZXRhaW5lZCBh bmQgZml4CisgICAgICAgIGluY29ycmVjdCBhc3N1bXB0aW9ucyBhYm91dCBob3cgd2VhayBoYW5k bGVzIHdvcmsgaW4gVjguIFRoaXMgaXMgcmVmYWN0b3JlZAorICAgICAgICBvdXQgb2YgdGhlIHBh dGNoIG9uIGh0dHA6Ly93a2J1Zy5jb20vOTM2NjEKKworICAgICAgICBObyB0ZXN0cyBuZWVkZWQs IHRoaXMganVzdCBmaXhlcyB0aGUgY29tcGlsZSBhbmQgd3JvbmcgdXNhZ2Ugb2YgU2NvcGVkUGVy c2lzdGVudC4KKworICAgICAgICAqIGJpbmRpbmdzL2pzL0pTRGVwZW5kZW50UmV0YWluZWQuaDoK KyAgICAgICAgKFdlYkNvcmU6OkpTRGVwZW5kZW50UmV0YWluZWQ6OkpTRGVwZW5kZW50UmV0YWlu ZWQpOgorICAgICAgICAoV2ViQ29yZTo6SlNEZXBlbmRlbnRSZXRhaW5lZDo6cmV0YWluKToKKyAg ICAgICAgKFdlYkNvcmU6OkpTRGVwZW5kZW50UmV0YWluZWQ6OnJlbGVhc2UpOgorICAgICAgICAo SlNEZXBlbmRlbnRSZXRhaW5lZCk6CisgICAgICAgICogYmluZGluZ3MvdjgvVjhEZXBlbmRlbnRS ZXRhaW5lZC5oOgorICAgICAgICAoV2ViQ29yZTo6VjhEZXBlbmRlbnRSZXRhaW5lZDo6VjhEZXBl bmRlbnRSZXRhaW5lZCk6CisgICAgICAgIChXZWJDb3JlOjpWOERlcGVuZGVudFJldGFpbmVkOjpy ZXRhaW4pOgorICAgICAgICAoV2ViQ29yZTo6VjhEZXBlbmRlbnRSZXRhaW5lZDo6d2Vha0NhbGxi YWNrKToKKwogMjAxMi0wOS0yOCAgQW5kZXJzIENhcmxzc29uICA8YW5kZXJzY2FAYXBwbGUuY29t PgogCiAgICAgICAgIFJlbW92ZSBJbnN0YW5jZTo6c2V0RGlkRXhlY3V0ZUZ1bmN0aW9uCmRpZmYg LS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU0RlcGVuZGVudFJldGFpbmVkLmgg Yi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU0RlcGVuZGVudFJldGFpbmVkLmgKaW5kZXgg ZmM0ZThmMzFmM2U0ZjZmYmY4MDBjMmJmYmYxYTkzNDY4OTYyNDMxMC4uZTgyODMzYTExODZjMDg5 NzhiYWVjODdlYTNkNmQ5MDU1ZDQ5NmJmMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvYmlu ZGluZ3MvanMvSlNEZXBlbmRlbnRSZXRhaW5lZC5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2JpbmRp bmdzL2pzL0pTRGVwZW5kZW50UmV0YWluZWQuaApAQCAtMjcsNiArMjcsNyBAQAogI2lmbmRlZiBK U0RlcGVuZGVudFJldGFpbmVkX2gKICNkZWZpbmUgSlNEZXBlbmRlbnRSZXRhaW5lZF9oCiAKKyNp bmNsdWRlICJKU0RPTUdsb2JhbE9iamVjdC5oIgogI2luY2x1ZGUgPGhlYXAvV2Vhay5oPgogI2lu Y2x1ZGUgPHJ1bnRpbWUvSlNPYmplY3QuaD4KICNpbmNsdWRlIDxydW50aW1lL1ByaXZhdGVOYW1l Lmg+CkBAIC0zNiw4ICszNyw5IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAogY2xhc3MgSlNEZXBl bmRlbnRSZXRhaW5lZCB7CiBwdWJsaWM6Ci0gICAgSlNEZXBlbmRlbnRSZXRhaW5lZChKU0M6OkpT T2JqZWN0KiBvd25lciwgSlNDOjpKU09iamVjdCogdmFsdWUpCisgICAgSlNEZXBlbmRlbnRSZXRh aW5lZChKU0M6OkpTT2JqZWN0KiBvd25lciwgSlNDOjpKU09iamVjdCogdmFsdWUsIEpTRE9NR2xv YmFsT2JqZWN0KiBnbG9iYWxPYmplY3QpCiAgICAgICAgIDogbV92YWx1ZSh2YWx1ZSkKKyAgICAg ICAgLCBtX2dsb2JhbE9iamVjdChnbG9iYWxPYmplY3QpCiAgICAgewogICAgICAgICBBU1NFUlQo dmFsdWUpOwogICAgICAgICBpZiAob3duZXIpCkBAIC02NCw3ICs2Niw3IEBAIHB1YmxpYzoKICAg ICAgICAgQVNTRVJUKCFtX293bmVyICYmIG93bmVyKTsKICAgICAgICAgQVNTRVJUKG1fdmFsdWUp OwogICAgICAgICBtX293bmVyID0gSlNDOjpQYXNzV2VhazxKU0M6OkpTT2JqZWN0Pihvd25lcik7 Ci0gICAgICAgIG1fb3duZXItPnB1dERpcmVjdChtX3Byb3BlcnR5TmFtZSwgZ2V0KCkpOworICAg ICAgICBtX293bmVyLT5wdXREaXJlY3QobV9nbG9iYWxPYmplY3QtPmdsb2JhbERhdGEoKSwgbV9w cm9wZXJ0eU5hbWUsIGdldCgpKTsKICAgICB9CiAKIHByaXZhdGU6CkBAIC03Miw3ICs3NCw3IEBA IHByaXZhdGU6CiAgICAgdm9pZCByZWxlYXNlKCkKICAgICB7CiAgICAgICAgIGlmIChtX293bmVy KQotICAgICAgICAgICAgbV9vd25lci0+cmVtb3ZlRGlyZWN0KG1fcHJvcGVydHlOYW1lKTsKKyAg ICAgICAgICAgIG1fb3duZXItPnJlbW92ZURpcmVjdChtX2dsb2JhbE9iamVjdC0+Z2xvYmFsRGF0 YSgpLCBtX3Byb3BlcnR5TmFtZSk7CiAgICAgICAgIG1fdmFsdWUuY2xlYXIoKTsKICAgICAgICAg bV9vd25lci5jbGVhcigpOwogICAgIH0KQEAgLTgwLDYgKzgyLDcgQEAgcHJpdmF0ZToKICAgICBK U0M6OldlYWs8SlNDOjpKU09iamVjdD4gbV9vd25lcjsKICAgICBKU0M6OldlYWs8SlNDOjpKU09i amVjdD4gbV92YWx1ZTsKICAgICBKU0M6OlByaXZhdGVOYW1lIG1fcHJvcGVydHlOYW1lOworICAg IEpTQzo6V2VhazxKU0RPTUdsb2JhbE9iamVjdD4gbV9nbG9iYWxPYmplY3Q7CiB9OwogCiB9IC8v IG5hbWVzcGFjZSBXZWJDb3JlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92 OC9WOERlcGVuZGVudFJldGFpbmVkLmggYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9WOERl cGVuZGVudFJldGFpbmVkLmgKaW5kZXggZDc4NzYxMjUyYTM5MGI3YmVmMTVlYWFkNjNkMDc4ODFl OThjOTc4Ny4uNjI3NzAwYmFjYTY4NzE2MWJjNTNlYmU2Njc3OTczNjBjNzIyMGRhYyAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhEZXBlbmRlbnRSZXRhaW5lZC5oCisr KyBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RGVwZW5kZW50UmV0YWluZWQuaApAQCAt MzQsNiArMzQsNyBAQAogI2luY2x1ZGUgPHd0Zi9Gb3J3YXJkLmg+CiAjaW5jbHVkZSA8d3RmL1Vu dXNlZFBhcmFtLmg+CiAjaW5jbHVkZSA8d3RmL2R0b2EuaD4KKyNpbmNsdWRlIDx3dGYvdGV4dC9T dHJpbmdCdWlsZGVyLmg+CiAKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIApAQCAtNDQsNyArNDUsNyBA QCBwdWJsaWM6CiAgICAgICAgICwgbV9wcm9wZXJ0eU5hbWUoY3JlYXRlUHJvcGVydHlOYW1lKCkp CiAgICAgewogICAgICAgICBBU1NFUlQoIW1fdmFsdWUuaXNFbXB0eSgpKTsKLSAgICAgICAgbV92 YWx1ZS5nZXQoKS5NYWtlV2Vhayh0aGlzLCAmVjhEZXBlbmRlbnRSZXRhaW5lZDo6dmFsdWVXZWFr Q2FsbGJhY2spOworICAgICAgICBtX3ZhbHVlLmdldCgpLk1ha2VXZWFrKHRoaXMsICZWOERlcGVu ZGVudFJldGFpbmVkOjp3ZWFrQ2FsbGJhY2spOwogICAgICAgICBpZiAoIW93bmVyLklzRW1wdHko KSkKICAgICAgICAgICAgIHJldGFpbihvd25lcik7CiAgICAgfQpAQCAtNjksOCArNzAsOCBAQCBw dWJsaWM6CiAgICAgICAgIEFTU0VSVChtX293bmVyLmlzRW1wdHkoKSAmJiAhb3duZXIuSXNFbXB0 eSgpKTsKICAgICAgICAgQVNTRVJUKCFtX3ZhbHVlLmlzRW1wdHkoKSk7CiAgICAgICAgIG93bmVy LT5TZXRIaWRkZW5WYWx1ZShtX3Byb3BlcnR5TmFtZS5nZXQoKSwgZ2V0KCkpOwotICAgICAgICBt X293bmVyID0gb3duZXI7Ci0gICAgICAgIG1fb3duZXIuZ2V0KCkuTWFrZVdlYWsodGhpcywgJlY4 RGVwZW5kZW50UmV0YWluZWQ6Om93bmVyV2Vha0NhbGxiYWNrKTsKKyAgICAgICAgbV9vd25lci5z ZXQob3duZXIpOworICAgICAgICBtX293bmVyLmdldCgpLk1ha2VXZWFrKHRoaXMsICZWOERlcGVu ZGVudFJldGFpbmVkOjp3ZWFrQ2FsbGJhY2spOwogICAgIH0KIAogcHJpdmF0ZToKQEAgLTgyLDE5 ICs4MywxMiBAQCBwcml2YXRlOgogICAgICAgICByZXR1cm4gVjhIaWRkZW5Qcm9wZXJ0eU5hbWU6 OmhpZGRlblJlZmVyZW5jZU5hbWUocmVpbnRlcnByZXRfY2FzdDxjb25zdCBjaGFyKj4obmFtZS5j aGFyYWN0ZXJzOCgpKSwgbmFtZS5sZW5ndGgoKSwgTmV3U3RyaW5nKTsKICAgICB9CiAKLSAgICBz dGF0aWMgdm9pZCBvd25lcldlYWtDYWxsYmFjayh2ODo6UGVyc2lzdGVudDx2ODo6VmFsdWU+IG9i amVjdCwgdm9pZCogcGFyYW1ldGVyKQorICAgIHN0YXRpYyB2b2lkIHdlYWtDYWxsYmFjayh2ODo6 UGVyc2lzdGVudDx2ODo6VmFsdWU+IG9iamVjdCwgdm9pZCogcGFyYW1ldGVyKQogICAgIHsKICAg ICAgICAgVjhEZXBlbmRlbnRSZXRhaW5lZCogdmFsdWUgPSBzdGF0aWNfY2FzdDxWOERlcGVuZGVu dFJldGFpbmVkKj4ocGFyYW1ldGVyKTsKICAgICAgICAgdmFsdWUtPnJlbGVhc2UoKTsKICAgICB9 CiAKLSAgICBzdGF0aWMgdm9pZCB2YWx1ZVdlYWtDYWxsYmFjayh2ODo6UGVyc2lzdGVudDx2ODo6 VmFsdWU+IG9iamVjdCwgdm9pZCogcGFyYW1ldGVyKQotICAgIHsKLSAgICAgICAgVjhEZXBlbmRl bnRSZXRhaW5lZCogdmFsdWUgPSBzdGF0aWNfY2FzdDxWOERlcGVuZGVudFJldGFpbmVkKj4ocGFy YW1ldGVyKTsKLSAgICAgICAgLy8gVGhlIG93bmVyIGNhbGxiYWNrIHNob3VsZCBhbHdheXMgYmUg Y2FsbGVkIGZpcnN0LCBzaW5jZSBpdCByZXRhaW5zIHRoZSB2YWx1ZS4KLSAgICAgICAgQVNTRVJU X1VOVVNFRCh2YWx1ZSwgdmFsdWUtPmlzRW1wdHkoKSk7Ci0gICAgfQotCiAgICAgdm9pZCByZWxl YXNlKCkKICAgICB7CiAgICAgICAgIGlmICghbV9vd25lci5pc0VtcHR5KCkpCg==