Bug 27203 – Inspector: Remove Unintended Global Variables

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 32644 [details]
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.

[Joseph Pecoraro]

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. =)

[Patrick Mueller]

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.

[Patrick Mueller]

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.

[Joseph Pecoraro]

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?

[Patrick Mueller]

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

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45889.