32335 2009-12-09 11:35:59 -0800 [v8] in Debug builds create an additional handle scope when doing debug build checks 2009-12-11 10:50:05 -0800 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 antonm webkit-unassigned aroben commit-queue eric evan webkit.review.bot oldest_to_newest 170075 0 antonm 2009-12-09 11:35:59 -0800 As of now, v8::Object::GetPointerFromInternalField doesn't require handle scope. However, in Debug builds we do some checks before accessing it which require handle scope (as they use GetInternalField method, for example). Thus we need an additional handle scope in debug builds (only). 170083 1 44551 antonm 2009-12-09 11:46:35 -0800 Created attachment 44551 first take 170084 2 webkit.review.bot 2009-12-09 11:48:01 -0800 style-queue ran check-webkit-style on attachment 44551 without any errors. 170129 3 44551 abarth 2009-12-09 13:22:48 -0800 Comment on attachment 44551 first take Seems like the ASSERTS can be outside of the #ifdef. How much does it cost to create the handle scope unconditionally? I'd rather not have NDEBUG everywhere.... Can we create the handle scope at a lower level where we're already splitting on NDEBUG? 170401 4 antonm 2009-12-10 05:36:14 -0800 (In reply to comment #3) > (From update of attachment 44551 [details]) > Seems like the ASSERTS can be outside of the #ifdef. Technically yes. However, if I remember it right, one can compile WebKit w/ asserts, but in Release mode, so we'll be in the trouble anyway. Having said that, I'd easily adjust. > How much does it cost to create the handle scope unconditionally? I'd rather not have NDEBUG everywhere.... That's a hot path, and handle scope is only needed due to ASSERTS, so I'd rather not spend a CPU cycle on it. If you don't mind. > Can we create the handle scope at a lower level where we're > already splitting on NDEBUG? Yes, for maybeDOMWrapper, but there is this type check, alas. Overall, I wholeheartedly agree those NDEBUG are ugly. So just let me know how you'd like that to be arranged. 170444 5 44551 abarth 2009-12-10 09:05:26 -0800 Comment on attachment 44551 first take Ah! I misunderstood. It's the ASSERTS themselves that require the handle scope. Yeah, I don't see a more aesthetic way to do this. Thanks for explaining it to me. 170445 6 antonm 2009-12-10 09:07:54 -0800 (In reply to comment #5) > (From update of attachment 44551 [details]) > Ah! I misunderstood. It's the ASSERTS themselves that require the handle > scope. Yeah, I don't see a more aesthetic way to do this. Thanks for > explaining it to me. Thanks a lot for review, Adam, cq+'ing. 170446 7 44551 commit-queue 2009-12-10 09:19:17 -0800 Comment on attachment 44551 first take Rejecting patch 44551 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html 170454 8 44551 eric 2009-12-10 10:14:45 -0800 Comment on attachment 44551 first take Lets see if the failure reproduces. If it does, then I'll file a bug and look at the machine itself. 170455 9 44551 commit-queue 2009-12-10 10:21:47 -0800 Comment on attachment 44551 first take Rejecting patch 44551 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html 170493 10 eric 2009-12-10 12:55:50 -0800 I'm surprised the failure didn't occur earlier. It looks like svn-apply fails with this patch: bugzilla-tool apply-attachment 44551 I wonder if however you generated this git diff is different than how our scripts expect? 170732 11 antonm 2009-12-11 06:29:46 -0800 (In reply to comment #10) > I'm surprised the failure didn't occur earlier. It looks like svn-apply fails > with this patch: > > bugzilla-tool apply-attachment 44551 [details] > > I wonder if however you generated this git diff is different than how our > scripts expect? git diff --no-prefix Uploading a new patch (generated with plain git diff). Just in case: zsh/3 127 --% git --version git version 1.5.4.3 170733 12 44683 antonm 2009-12-11 06:30:33 -0800 Created attachment 44683 plain git diff 170734 13 webkit.review.bot 2009-12-11 06:33:21 -0800 style-queue ran check-webkit-style on attachment 44683 without any errors. 170778 14 44683 commit-queue 2009-12-11 09:14:15 -0800 Comment on attachment 44683 plain git diff Clearing flags on attachment: 44683 Committed r51995: <http://trac.webkit.org/changeset/51995> 170779 15 commit-queue 2009-12-11 09:14:20 -0800 All reviewed patches have been landed. Closing bug. 170780 16 antonm 2009-12-11 09:15:27 -0800 So yes, git diff --no-prefix was to blame. 170799 17 eric 2009-12-11 10:03:31 -0800 I've filed bug 32438 about the svn-apply bug. 170830 18 eric 2009-12-11 10:45:23 -0800 Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the "right thing" regarding git patches. See "bugzilla-tool help" for more information. You might find one of those commands useful to you when posting patches in the future. 170831 19 antonm 2009-12-11 10:50:05 -0800 (In reply to comment #18) > Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the > "right thing" regarding git patches. See "bugzilla-tool help" for more > information. You might find one of those commands useful to you when posting > patches in the future. Thank you very much for pointing it out---never heard of it. 44551 2009-12-09 11:46:35 -0800 2009-12-11 08:52:33 -0800 first take WC.patch text/plain 1605 antonm ZGlmZiAtLWdpdCBXZWJDb3JlL0NoYW5nZUxvZyBXZWJDb3JlL0NoYW5nZUxvZwppbmRleCBjOTI1 NzgwLi45ZTY1YzQzIDEwMDY0NAotLS0gV2ViQ29yZS9DaGFuZ2VMb2cKKysrIFdlYkNvcmUvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMDktMTItMDkgIGFudG9uIG11aGluICA8YW50b25t QGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBbdjhdIGluIERlYnVnIGJ1aWxkcyBjcmVhdGUgYW4gYWRkaXRpb25hbCBoYW5kbGUg c2NvcGUKKyAgICAgICAgd2hlbiBkb2luZyBkZWJ1ZyBidWlsZCBjaGVja3MuCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMjMzNQorCisgICAgICAgIFNo b3VsZCBiZSBjb3ZlcmVkIGJ5IGJ1aWxkYm90cy4KKworICAgICAgICAqIGJpbmRpbmdzL3Y4L1Y4 RE9NV3JhcHBlci5oOgorICAgICAgICAoV2ViQ29yZTo6VjhET01XcmFwcGVyOjpjb252ZXJ0RE9N V3JhcHBlclRvTmF0aXZlKToKKyAgICAgICAgKFdlYkNvcmU6OlY4RE9NV3JhcHBlcjo6Y29udmVy dERPTVdyYXBwZXJUb05vZGUpOgorCiAyMDA5LTEyLTA5ICBPbGl2ZXIgSHVudCAgPG9saXZlckBh cHBsZS5jb20+CiAKICAgICAgICAgQnVpbGQgZml4CmRpZmYgLS1naXQgV2ViQ29yZS9iaW5kaW5n cy92OC9WOERPTVdyYXBwZXIuaCBXZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NV3JhcHBlci5oCmlu ZGV4IGVmZjMyNTIuLmNlNTUwY2QgMTAwNjQ0Ci0tLSBXZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9N V3JhcHBlci5oCisrKyBXZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NV3JhcHBlci5oCkBAIC0xMjYs MTUgKzEyNiwyMSBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7CiAgICAgICAgIHRlbXBsYXRlIDxjbGFz cyBDPgogICAgICAgICBzdGF0aWMgQyogY29udmVydERPTVdyYXBwZXJUb05hdGl2ZSh2ODo6SGFu ZGxlPHY4OjpPYmplY3Q+IG9iamVjdCkKICAgICAgICAgeworI2lmbmRlZiBOREVCVUcKKyAgICAg ICAgICAgIHY4OjpIYW5kbGVTY29wZSBoYW5kbGVTY29wZTsKICAgICAgICAgICAgIEFTU0VSVCht YXliZURPTVdyYXBwZXIob2JqZWN0KSk7CisjZW5kaWYKICAgICAgICAgICAgIHJldHVybiByZWlu dGVycHJldF9jYXN0PEMqPihvYmplY3QtPkdldFBvaW50ZXJGcm9tSW50ZXJuYWxGaWVsZChWOEN1 c3RvbTo6a0RPTVdyYXBwZXJPYmplY3RJbmRleCkpOwogICAgICAgICB9CiAKICAgICAgICAgdGVt cGxhdGUgPGNsYXNzIEM+CiAgICAgICAgIHN0YXRpYyBDKiBjb252ZXJ0RE9NV3JhcHBlclRvTm9k ZSh2ODo6SGFuZGxlPHY4OjpPYmplY3Q+IG9iamVjdCkKICAgICAgICAgeworI2lmbmRlZiBOREVC VUcKKyAgICAgICAgICAgIHY4OjpIYW5kbGVTY29wZSBoYW5kbGVTY29wZTsKICAgICAgICAgICAg IEFTU0VSVChtYXliZURPTVdyYXBwZXIob2JqZWN0KSk7CiAgICAgICAgICAgICBBU1NFUlQoZG9t V3JhcHBlclR5cGUob2JqZWN0KSA9PSBWOENsYXNzSW5kZXg6Ok5PREUpOworI2VuZGlmCiAgICAg ICAgICAgICByZXR1cm4gY29udmVydERPTVdyYXBwZXJUb05hdGl2ZTxDPihvYmplY3QpOwogICAg ICAgICB9CiAK 44683 2009-12-11 06:30:33 -0800 2009-12-11 09:14:15 -0800 plain git diff WC.patch text/plain 1686 antonm ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NzNhMGJkYS4uOTQyNTc5OSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAwOS0xMi0wOSAgYW50b24gbXVoaW4g IDxhbnRvbm1AY2hyb21pdW0ub3JnPgorICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBbdjhdIGluIERlYnVnIGJ1aWxkcyBjcmVhdGUgYW4gYWRkaXRpb25hbCBo YW5kbGUgc2NvcGUKKyAgICAgICAgd2hlbiBkb2luZyBkZWJ1ZyBidWlsZCBjaGVja3MuCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMjMzNQorCisgICAg ICAgIFNob3VsZCBiZSBjb3ZlcmVkIGJ5IGJ1aWxkYm90cy4KKworICAgICAgICAqIGJpbmRpbmdz L3Y4L1Y4RE9NV3JhcHBlci5oOgorICAgICAgICAoV2ViQ29yZTo6VjhET01XcmFwcGVyOjpjb252 ZXJ0RE9NV3JhcHBlclRvTmF0aXZlKToKKyAgICAgICAgKFdlYkNvcmU6OlY4RE9NV3JhcHBlcjo6 Y29udmVydERPTVdyYXBwZXJUb05vZGUpOgorCiAyMDA5LTEyLTExICBTaW1vbiBIYXVzbWFubiAg PGhhdXNtYW5uQHdlYmtpdC5vcmc+LCBLaW0gR3LDtm5ob2xtICA8a2ltLmdyb25ob2xtQG5vbW92 b2suY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFudHRpIEtvaXZpc3RvLgpkaWZmIC0tZ2l0 IGEvV2ViQ29yZS9iaW5kaW5ncy92OC9WOERPTVdyYXBwZXIuaCBiL1dlYkNvcmUvYmluZGluZ3Mv djgvVjhET01XcmFwcGVyLmgKaW5kZXggZWZmMzI1Mi4uY2U1NTBjZCAxMDA2NDQKLS0tIGEvV2Vi Q29yZS9iaW5kaW5ncy92OC9WOERPTVdyYXBwZXIuaAorKysgYi9XZWJDb3JlL2JpbmRpbmdzL3Y4 L1Y4RE9NV3JhcHBlci5oCkBAIC0xMjYsMTUgKzEyNiwyMSBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7 CiAgICAgICAgIHRlbXBsYXRlIDxjbGFzcyBDPgogICAgICAgICBzdGF0aWMgQyogY29udmVydERP TVdyYXBwZXJUb05hdGl2ZSh2ODo6SGFuZGxlPHY4OjpPYmplY3Q+IG9iamVjdCkKICAgICAgICAg eworI2lmbmRlZiBOREVCVUcKKyAgICAgICAgICAgIHY4OjpIYW5kbGVTY29wZSBoYW5kbGVTY29w ZTsKICAgICAgICAgICAgIEFTU0VSVChtYXliZURPTVdyYXBwZXIob2JqZWN0KSk7CisjZW5kaWYK ICAgICAgICAgICAgIHJldHVybiByZWludGVycHJldF9jYXN0PEMqPihvYmplY3QtPkdldFBvaW50 ZXJGcm9tSW50ZXJuYWxGaWVsZChWOEN1c3RvbTo6a0RPTVdyYXBwZXJPYmplY3RJbmRleCkpOwog ICAgICAgICB9CiAKICAgICAgICAgdGVtcGxhdGUgPGNsYXNzIEM+CiAgICAgICAgIHN0YXRpYyBD KiBjb252ZXJ0RE9NV3JhcHBlclRvTm9kZSh2ODo6SGFuZGxlPHY4OjpPYmplY3Q+IG9iamVjdCkK ICAgICAgICAgeworI2lmbmRlZiBOREVCVUcKKyAgICAgICAgICAgIHY4OjpIYW5kbGVTY29wZSBo YW5kbGVTY29wZTsKICAgICAgICAgICAgIEFTU0VSVChtYXliZURPTVdyYXBwZXIob2JqZWN0KSk7 CiAgICAgICAgICAgICBBU1NFUlQoZG9tV3JhcHBlclR5cGUob2JqZWN0KSA9PSBWOENsYXNzSW5k ZXg6Ok5PREUpOworI2VuZGlmCiAgICAgICAgICAgICByZXR1cm4gY29udmVydERPTVdyYXBwZXJU b05hdGl2ZTxDPihvYmplY3QpOwogICAgICAgICB9CiAK