27203 2009-07-12 21:27:22 -0700 Inspector: Remove Unintended Global Variables 2009-07-14 22:26:58 -0700 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) All OS X 10.5 RESOLVED FIXED P2 Normal --- 0 joepeck webkit-unassigned bfulgham pmuellr oldest_to_newest 131239 0 joepeck 2009-07-12 21:27:22 -0700 Removing some variables that were declared without 'var' and thus pushed into the global scope by accident. Some in the Web Inspector window only (of little important but to be clean), but some in the actual inspected window as well (may give developers problems). I did some hunting all over and these were the ones I found. 131240 1 32644 joepeck 2009-07-12 21:40:20 -0700 Created attachment 32644 Remove Unintended Globals This removes the unintended globals that I found. Most are harmless and are only global variables inside the Web Inspector window. Note that in the case of Console.js, there is code that gets evaluated in the inspected window. Thus the "k" variable could get overwritten when running either the keys() or values() functions created by the Web Inspector. For example, running some sample code in the Inspector: > k ReferenceError: Can't find variable: k > var obj = { a:1 } undefined > keys(obj) ["a"] > k /* Now defined as a side effect of running keys() */ a Although its probably bad programming if a developer used window.k, and even more unusual if they then called these functions, this side effect should still be removed. =) Cheers. 131243 2 joepeck 2009-07-12 22:23:06 -0700 Just a heads up, I ran some scripts to check the differences of the global scope of a Web Inspector window and a normal global scope. I also removed all the expected global functions from inspector.js and utilities.js and I was left with the following leftovers: > ["WebInspector", "localizedStrings", "UserInitiatedProfileName", "TreeElement", "TreeOutline", "Preferences", "InspectorController", "key", "subPropertyName"] "key" and "subPropertyName" are now squashed. And I assume the rest are expected to be in the global scope. Let me know if any of these should be moved, but I think its all good now. =) 131401 3 pmuellr 2009-07-13 13:36:19 -0700 It would be useful to have a testcase that could be run to actually test for the presence of new unintended globals introduced. It would probably need to be manual. Test page would capture existing globals, tell the user to open web inspector, run some code, and then have a link/button on the test page that would compare the new state of the globals to the initial captured state. 131402 4 pmuellr 2009-07-13 13:40:45 -0700 I'm curious as to whether, in addition to "window" (the global object), if Web Inspector is adding properties to any other objects. Even if it isn't today, perhaps we should start proactively checking obvious places, like existing classes and their prototypes. 131405 5 joepeck 2009-07-13 13:59:02 -0700 As far as modifying anything on the actually inspected page I have doubts that anything else is affected. From what I've seen, code in the Inspector is separate from the inspected page and anything to go in the inspected page needs to be specially evaluted. The correction above is the only place where this is done with any significance. However, this could be useful to clean up Javascript inside the Inspector page or better yet javascript in general. Does WebKit have the ability to do a JSLint like compilation of a Javascript file and identify potential problems such as "implied globals" or access outside of an existing scope? Seeing as there are JIT compilers then this kind of static analysis would be useful for developers. I haven't written an test cases before. I could look into doing something like this (no promises yet). The problem I see with a test case for this is that the inspector adds a lot of functions on html/dom prototypes (see utilities.js). If any more are created or modified then the test-case should be corrected. Is that kind of dependency normal for test cases? 131438 6 pmuellr 2009-07-13 16:28:26 -0700 As I mentioned, checking for properties added to objects other than "window" is more of a proactive test. I assume. :-) Are the functions added in utilities.js added to the under-test JavaScript environment? Seems like a huge problem; something that itself needs to be cleaned up, somehow. Or minimized. Or all the functions should have double underscores added to them. In terms of test cases, manual ones aren't bad. See one I wrote: WebKit/WebCore/manual-tests/inspector/named-evals.html 131811 7 bfulgham 2009-07-14 22:26:58 -0700 Landed in http://trac.webkit.org/changeset/45889. 32644 2009-07-12 21:40:20 -0700 2009-07-12 21:57:48 -0700 Remove Unintended Globals InspectorRemoveGlobals.patch text/plain 3867 joepeck SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NTc4NSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTkgQEAKKzIwMDktMDctMTIgIEpvc2VwaCBQZWNvcmFybyAgPGpvZXBlY2swMkBn bWFpbC5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgSW5zcGVjdG9yOiBSZW1vdmUgVW5pbnRlbmRlZCBHbG9iYWwgVmFyaWFibGVzCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yNzIwMworCisgICAgICAg ICogaW5zcGVjdG9yL2Zyb250LWVuZC9Db25zb2xlLmpzOgorICAgICAgICAoV2ViSW5zcGVjdG9y LkNvbnNvbGUucHJvdG90eXBlLl9lbnN1cmVDb21tYW5kTGluZUFQSUluc3RhbGxlZCk6CisgICAg ICAgICogaW5zcGVjdG9yL2Zyb250LWVuZC9EYXRhYmFzZXNQYW5lbC5qczoKKyAgICAgICAgKFdl Ykluc3BlY3Rvci5EYXRhYmFzZXNQYW5lbC5wcm90b3R5cGUuZGF0YUdyaWRGb3JET01TdG9yYWdl KToKKyAgICAgICAgKiBpbnNwZWN0b3IvZnJvbnQtZW5kL09iamVjdFByb3BlcnRpZXNTZWN0aW9u LmpzOgorICAgICAgICAoV2ViSW5zcGVjdG9yLk9iamVjdFByb3BlcnR5VHJlZUVsZW1lbnQucHJv dG90eXBlLnVwZGF0ZSk6CisgICAgICAgICogaW5zcGVjdG9yL2Zyb250LWVuZC9pbnNwZWN0b3Iu anM6CisgICAgICAgIChXZWJJbnNwZWN0b3IuYW5pbWF0ZVN0eWxlKToKKwogMjAwOS0wNy0xMiAg TmF0ZSBDaGFwaW4gIDxqYXBoZXRAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5 IERpbWl0cmkgR2xhemtvdi4KSW5kZXg6IFdlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9Db25z b2xlLmpzCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9Db25zb2xl LmpzCShyZXZpc2lvbiA0NTc4NSkKKysrIFdlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9Db25z b2xlLmpzCSh3b3JraW5nIGNvcHkpCkBAIC00MDksOCArNDA5LDggQEAgV2ViSW5zcGVjdG9yLkNv bnNvbGUucHJvdG90eXBlID0gewogICAgICAgICAgICAgICAgIH0sIFwKICAgICAgICAgICAgICAg ICBkaXI6IGZ1bmN0aW9uKCkgeyByZXR1cm4gY29uc29sZS5kaXIuYXBwbHkoY29uc29sZSwgYXJn dW1lbnRzKSB9LCBcCiAgICAgICAgICAgICAgICAgZGlyeG1sOiBmdW5jdGlvbigpIHsgcmV0dXJu IGNvbnNvbGUuZGlyeG1sLmFwcGx5KGNvbnNvbGUsIGFyZ3VtZW50cykgfSwgXAotICAgICAgICAg ICAgICAgIGtleXM6IGZ1bmN0aW9uKG8pIHsgdmFyIGEgPSBbXTsgZm9yIChrIGluIG8pIGEucHVz aChrKTsgcmV0dXJuIGE7IH0sIFwKLSAgICAgICAgICAgICAgICB2YWx1ZXM6IGZ1bmN0aW9uKG8p IHsgdmFyIGEgPSBbXTsgZm9yIChrIGluIG8pIGEucHVzaChvW2tdKTsgcmV0dXJuIGE7IH0sIFwK KyAgICAgICAgICAgICAgICBrZXlzOiBmdW5jdGlvbihvKSB7IHZhciBhID0gW107IGZvciAodmFy IGsgaW4gbykgYS5wdXNoKGspOyByZXR1cm4gYTsgfSwgXAorICAgICAgICAgICAgICAgIHZhbHVl czogZnVuY3Rpb24obykgeyB2YXIgYSA9IFtdOyBmb3IgKHZhciBrIGluIG8pIGEucHVzaChvW2td KTsgcmV0dXJuIGE7IH0sIFwKICAgICAgICAgICAgICAgICBwcm9maWxlOiBmdW5jdGlvbigpIHsg cmV0dXJuIGNvbnNvbGUucHJvZmlsZS5hcHBseShjb25zb2xlLCBhcmd1bWVudHMpIH0sIFwKICAg ICAgICAgICAgICAgICBwcm9maWxlRW5kOiBmdW5jdGlvbigpIHsgcmV0dXJuIGNvbnNvbGUucHJv ZmlsZUVuZC5hcHBseShjb25zb2xlLCBhcmd1bWVudHMpIH0gXAogICAgICAgICAgICAgfTsiKTsK SW5kZXg6IFdlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9EYXRhYmFzZXNQYW5lbC5qcwo9PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09Ci0tLSBXZWJDb3JlL2luc3BlY3Rvci9mcm9udC1lbmQvRGF0YWJhc2VzUGFuZWwuanMJ KHJldmlzaW9uIDQ1Nzg1KQorKysgV2ViQ29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL0RhdGFiYXNl c1BhbmVsLmpzCSh3b3JraW5nIGNvcHkpCkBAIC0zMzIsNyArMzMyLDcgQEAgV2ViSW5zcGVjdG9y LkRhdGFiYXNlc1BhbmVsLnByb3RvdHlwZSA9IAogICAgICAgICB2YXIgbm9kZXMgPSBbXTsKICAg ICAgICAgCiAgICAgICAgIHZhciBsZW5ndGggPSBkb21TdG9yYWdlLmxlbmd0aDsKLSAgICAgICAg Zm9yIChpbmRleCA9IDA7IGluZGV4IDwgZG9tU3RvcmFnZS5sZW5ndGg7IGluZGV4KyspIHsKKyAg ICAgICAgZm9yICh2YXIgaW5kZXggPSAwOyBpbmRleCA8IGRvbVN0b3JhZ2UubGVuZ3RoOyBpbmRl eCsrKSB7CiAgICAgICAgICAgICB2YXIgZGF0YSA9IHt9OwogICAgICAgIAogICAgICAgICAgICAg dmFyIGtleSA9IFN0cmluZyhkb21TdG9yYWdlLmtleShpbmRleCkpOwpJbmRleDogV2ViQ29yZS9p bnNwZWN0b3IvZnJvbnQtZW5kL09iamVjdFByb3BlcnRpZXNTZWN0aW9uLmpzCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFdlYkNvcmUvaW5zcGVjdG9yL2Zyb250LWVuZC9PYmplY3RQcm9wZXJ0aWVzU2VjdGlvbi5q cwkocmV2aXNpb24gNDU3ODUpCisrKyBXZWJDb3JlL2luc3BlY3Rvci9mcm9udC1lbmQvT2JqZWN0 UHJvcGVydGllc1NlY3Rpb24uanMJKHdvcmtpbmcgY29weSkKQEAgLTE1Niw3ICsxNTYsNyBAQCBX ZWJJbnNwZWN0b3IuT2JqZWN0UHJvcGVydHlUcmVlRWxlbWVudC5wCiAgICAgICAgIHZhciBoYXNT dWJQcm9wZXJ0aWVzID0gZmFsc2U7CiAgICAgICAgIHZhciB0eXBlID0gdHlwZW9mIGNoaWxkT2Jq ZWN0OwogICAgICAgICBpZiAoY2hpbGRPYmplY3QgJiYgKHR5cGUgPT09ICJvYmplY3QiIHx8IHR5 cGUgPT09ICJmdW5jdGlvbiIpKSB7Ci0gICAgICAgICAgICBmb3IgKHN1YlByb3BlcnR5TmFtZSBp biBjaGlsZE9iamVjdCkgeworICAgICAgICAgICAgZm9yICh2YXIgc3ViUHJvcGVydHlOYW1lIGlu IGNoaWxkT2JqZWN0KSB7CiAgICAgICAgICAgICAgICAgaWYgKHN1YlByb3BlcnR5TmFtZSA9PT0g Il9fdHJlZUVsZW1lbnRJZGVudGlmaWVyIikKICAgICAgICAgICAgICAgICAgICAgY29udGludWU7 CiAgICAgICAgICAgICAgICAgaGFzU3ViUHJvcGVydGllcyA9IHRydWU7CkluZGV4OiBXZWJDb3Jl L2luc3BlY3Rvci9mcm9udC1lbmQvaW5zcGVjdG9yLmpzCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUv aW5zcGVjdG9yL2Zyb250LWVuZC9pbnNwZWN0b3IuanMJKHJldmlzaW9uIDQ1Nzg1KQorKysgV2Vi Q29yZS9pbnNwZWN0b3IvZnJvbnQtZW5kL2luc3BlY3Rvci5qcwkod29ya2luZyBjb3B5KQpAQCAt NTg4LDYgKzU4OCw3IEBAIFdlYkluc3BlY3Rvci5hbmltYXRlU3R5bGUgPSBmdW5jdGlvbihhbmkK ICAgICAgICAgdmFyIHN0YXJ0ID0gbnVsbDsKICAgICAgICAgdmFyIGN1cnJlbnQgPSBudWxsOwog ICAgICAgICB2YXIgZW5kID0gbnVsbDsKKyAgICAgICAgdmFyIGtleSA9IG51bGw7CiAgICAgICAg IGZvciAoa2V5IGluIGFuaW1hdGlvbikgewogICAgICAgICAgICAgaWYgKGtleSA9PT0gImVsZW1l bnQiKQogICAgICAgICAgICAgICAgIGVsZW1lbnQgPSBhbmltYXRpb25ba2V5XTsK