137906 2014-10-20 18:29:03 -0700 Don't create cached functions for HTMLDocument.write*() 2014-10-29 11:32:45 -0700 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 136724 136901 137907 1 msaboff webkit-unassigned darin ggaren simon.fraser oldest_to_newest 1043054 0 msaboff 2014-10-20 18:29:03 -0700 For the same reasons outline in <https://bugs.webkit.org/show_bug.cgi?id=137839> - "Don't create cached functions that access lexicalGlobalObject()", create uncached version of write() and writeln(). These function should have been part of that bug, but are handled here. 1043063 1 240172 msaboff 2014-10-20 18:50:50 -0700 Created attachment 240172 Patch 1043181 2 240172 darin 2014-10-21 08:44:16 -0700 Comment on attachment 240172 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240172&action=review > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:69 > + if (equal(propertyName.publicName(), "write")) { Given that this idiom is a good one, I think we should add: bool operator==(PropertyName, const char*); So we don't have to write this longer sequence at each call site. I also think it’s possibly slightly more efficient to save one branch and use uid() instead of publicName() since the empty uniques would never compare equal to a non-empty string. But that's the kind of thing that I would like the person writing the override to worry about rather than having to worry about it at each call site. 1043187 3 msaboff 2014-10-21 08:52:03 -0700 (In reply to comment #2) > Comment on attachment 240172 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240172&action=review > > > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:69 > > + if (equal(propertyName.publicName(), "write")) { > > Given that this idiom is a good one, I think we should add: > > bool operator==(PropertyName, const char*); > > So we don't have to write this longer sequence at each call site. I also > think it’s possibly slightly more efficient to save one branch and use uid() > instead of publicName() since the empty uniques would never compare equal to > a non-empty string. But that's the kind of thing that I would like the > person writing the override to worry about rather than having to worry about > it at each call site. Thanks for the review. I'll make the suggested changes, including using uid() in another patch. Tracked in <https://bugs.webkit.org/show_bug.cgi?id=137925> - "Add operator==(PropertyName, char*)" 1043215 4 msaboff 2014-10-21 10:19:45 -0700 Committed r174985: <http://trac.webkit.org/changeset/174985> 1043225 5 ggaren 2014-10-21 10:47:02 -0700 This is the kind of subtle change where the test case is more valuable than the code change. Please add a test for this, or note in the ChangeLog which tests already cover this. 1043234 6 msaboff 2014-10-21 11:12:02 -0700 (In reply to comment #5) > This is the kind of subtle change where the test case is more valuable than > the code change. Please add a test for this, or note in the ChangeLog which > tests already cover this. This change is covered by the tests: LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-write-lexical.html LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-writeln-lexical.html LayoutTests/http/tests/security/aboutBlank/security-context-write.html LayoutTests/http/tests/security/aboutBlank/security-context-writeln.html LayoutTests/http/tests/xmlhttprequest/request-from-popup.html LayoutTests/http/tests/navigation/new-window-redirect-history.html LayoutTests/http/tests/misc/window-open-then-write.html I'll update the change log accordingly. 1043273 7 darin 2014-10-21 13:59:41 -0700 (In reply to comment #6) > This change is covered by the tests: Sorry, I don’t understand. Were those tests failing before? I didn’t see a patch to the expected results to expect success instead of failure. 1043278 8 msaboff 2014-10-21 14:11:02 -0700 (In reply to comment #7) > (In reply to comment #6) > > This change is covered by the tests: > > Sorry, I don’t understand. Were those tests failing before? I didn’t see a > patch to the expected results to expect success instead of failure. This change was split out from <https://bugs.webkit.org/show_bug.cgi?id=137907> similar to how <https://bugs.webkit.org/show_bug.cgi?id=137839> was split out. This change is needed for <https://bugs.webkit.org/show_bug.cgi?id=137907> or the tests listed above fail. 1044792 9 simon.fraser 2014-10-29 11:32:45 -0700 I think this caused bug 138082. 240172 2014-10-20 18:50:50 -0700 2014-10-21 08:44:16 -0700 Patch 137906.patch text/plain 2753 msaboff SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE3NDkxOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE0LTEwLTIwICBNaWNoYWVs IFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIERvbid0IGNyZWF0ZSBjYWNo ZWQgZnVuY3Rpb25zIHRoYXQgYWNjZXNzIGxleGljYWxHbG9iYWxPYmplY3QoKQorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTM3ODM5CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWFkZSBpdCBzbyB0aGF0IEpT SFRNTERvY3VtZW50Ojp3cml0ZSgpIGFuZCA6OndyaXRlbG4oKSBhcmUgYWx3YXlzIGNyZWF0ZWQg Zm9yIGV2ZXJ5IHByb3RvdHlwZS4KKyAgICAgICAgVGhpcyBpcyBzaW1pbGFyIHRvIHRoZSBjaGFu Z2UgaW4gcjE3NDg0Ny4KKworICAgICAgICAqIGJpbmRpbmdzL2pzL0pTSFRNTERvY3VtZW50Q3Vz dG9tLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkpTSFRNTERvY3VtZW50OjpnZXRPd25Qcm9wZXJ0 eVNsb3QpOgorICAgICAgICAqIGh0bWwvSFRNTERvY3VtZW50LmlkbDoKKwogMjAxNC0xMC0yMCAg TWljaGFlbCBTYWJvZmYgIDxtc2Fib2ZmQGFwcGxlLmNvbT4KIAogICAgICAgICBNYWtlIHBvc3Qg Y2hlY2tpbiBzdWdnZXN0ZWQgY2hhbmdlcyB0byByMTc0ODQ3CkluZGV4OiBTb3VyY2UvV2ViQ29y ZS9iaW5kaW5ncy9qcy9KU0hUTUxEb2N1bWVudEN1c3RvbS5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvSlNIVE1MRG9jdW1lbnRDdXN0b20uY3BwCShyZXZpc2lv biAxNzQ5MTgpCisrKyBTb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU0hUTUxEb2N1bWVudEN1 c3RvbS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTY2LDYgKzY2LDE0IEBAIGJvb2wgSlNIVE1MRG9j dW1lbnQ6OmdldE93blByb3BlcnR5U2xvdCgKICAgICAgICAgc2xvdC5zZXRDdXN0b20odGhpc09i amVjdCwgUmVhZE9ubHkgfCBEb250RGVsZXRlIHwgRG9udEVudW0sIG5vbkNhY2hpbmdTdGF0aWNG dW5jdGlvbkdldHRlcjxqc0hUTUxEb2N1bWVudFByb3RvdHlwZUZ1bmN0aW9uT3BlbiwgMj4pOwog ICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICB9CisgICAgaWYgKGVxdWFsKHByb3BlcnR5TmFtZS5w dWJsaWNOYW1lKCksICJ3cml0ZSIpKSB7CisgICAgICAgIHNsb3Quc2V0Q3VzdG9tKHRoaXNPYmpl Y3QsIFJlYWRPbmx5IHwgRG9udERlbGV0ZSB8IERvbnRFbnVtLCBub25DYWNoaW5nU3RhdGljRnVu Y3Rpb25HZXR0ZXI8anNIVE1MRG9jdW1lbnRQcm90b3R5cGVGdW5jdGlvbldyaXRlLCAxPik7Cisg ICAgICAgIHJldHVybiB0cnVlOworICAgIH0KKyAgICBpZiAoZXF1YWwocHJvcGVydHlOYW1lLnB1 YmxpY05hbWUoKSwgIndyaXRlbG4iKSkgeworICAgICAgICBzbG90LnNldEN1c3RvbSh0aGlzT2Jq ZWN0LCBSZWFkT25seSB8IERvbnREZWxldGUgfCBEb250RW51bSwgbm9uQ2FjaGluZ1N0YXRpY0Z1 bmN0aW9uR2V0dGVyPGpzSFRNTERvY3VtZW50UHJvdG90eXBlRnVuY3Rpb25Xcml0ZWxuLCAxPik7 CisgICAgICAgIHJldHVybiB0cnVlOworICAgIH0KIAogICAgIGlmIChjYW5HZXRJdGVtc0Zvck5h bWUoZXhlYywgJnRoaXNPYmplY3QtPmltcGwoKSwgcHJvcGVydHlOYW1lKSkgewogICAgICAgICBz bG90LnNldEN1c3RvbSh0aGlzT2JqZWN0LCBSZWFkT25seSB8IERvbnREZWxldGUgfCBEb250RW51 bSwgdGhpc09iamVjdC0+bmFtZUdldHRlcik7CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hU TUxEb2N1bWVudC5pZGwKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRG9j dW1lbnQuaWRsCShyZXZpc2lvbiAxNzQ5MTcpCisrKyBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxE b2N1bWVudC5pZGwJKHdvcmtpbmcgY29weSkKQEAgLTI1LDggKzI1LDggQEAKIF0gaW50ZXJmYWNl IEhUTUxEb2N1bWVudCA6IERvY3VtZW50IHsKICAgICBbQ3VzdG9tLCBGb3J3YXJkRGVjbGFyZUlu SGVhZGVyXSB2b2lkIG9wZW4oKTsKICAgICB2b2lkIGNsb3NlKCk7Ci0gICAgW0N1c3RvbV0gdm9p ZCB3cml0ZShbRGVmYXVsdD1VbmRlZmluZWRdIG9wdGlvbmFsIERPTVN0cmluZyB0ZXh0KTsKLSAg ICBbQ3VzdG9tXSB2b2lkIHdyaXRlbG4oW0RlZmF1bHQ9VW5kZWZpbmVkXSBvcHRpb25hbCBET01T dHJpbmcgdGV4dCk7CisgICAgW0N1c3RvbSwgRm9yd2FyZERlY2xhcmVJbkhlYWRlcl0gdm9pZCB3 cml0ZShbRGVmYXVsdD1VbmRlZmluZWRdIG9wdGlvbmFsIERPTVN0cmluZyB0ZXh0KTsKKyAgICBb Q3VzdG9tLCBGb3J3YXJkRGVjbGFyZUluSGVhZGVyXSB2b2lkIHdyaXRlbG4oW0RlZmF1bHQ9VW5k ZWZpbmVkXSBvcHRpb25hbCBET01TdHJpbmcgdGV4dCk7CiAKICAgICByZWFkb25seSBhdHRyaWJ1 dGUgSFRNTENvbGxlY3Rpb24gZW1iZWRzOwogICAgIHJlYWRvbmx5IGF0dHJpYnV0ZSBIVE1MQ29s bGVjdGlvbiBwbHVnaW5zOwo=