621644 - $ is shadowed in web console

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Ian Bicking (:ianbicking)]

When using a page with jQuery, $() in the Web Console is the native Web Console implementation (JSTH_$ which acts like document.getElementById).  This is very jarring/confusing.  If $ is already defined it shouldn't be redefined.

Comment 1

[Kevin Dangoor]

It's also potentially jarring and confusing if the Web Console does not behave consistently between pages. For example, if you generally use jQuery but go to a page that uses Prototype, the $() function would then behave as Prototype's does. $() in the Web Console is at least consistent whenever you use the console.

Note that you can still use jQuery by using window.$

There are certainly arguments to be made both ways.

Comment 2

[Ian Bicking (:ianbicking)]

Another issue: I was only able to infer $ even existed and was defined by the console after encountering this problem.  Otherwise I never noticed it existed, and I don't know exactly how I'd have discovered it existed (even if there were docs for this, I wouldn't have read them ;).  I only figured out what it was by looking at the $ object, finding the string JSTH_$, and googling for that.  If the repr for $ was something more like "WebConsole_$" I might have figured it out a lot more easily (though how I'd have figured out window.$, I don't know).

I don't think differing definitions of $ is at all a problem except when you are poking around other people's pages -- when I'm debugging pages of my own making (so far primarily what I've used Web Console for) I know and want to interact with the Javascript environment I've created.

Comment 3

[Kevin Dangoor]

try typing "help"  (though I do wonder how many people will do that?)

You do make a good point about the fact that most people will use the Web Console on their own pages, most of the time and therefore know how $ is supposed to act.

Comment 4

[Ian Bicking (:ianbicking)]

"help" never occurred to me.  I've mostly been using it as though it is basically the same as Firebug and Chrome's consoles, and anything that doesn't work like I expect I then mostly avoid (for better or worse)

Comment 5

[Kevin Dangoor]

OK, I'm convinced. I'll work on getting this fixed in Firefox 4.

Comment 6

[David Dahl :ddahl]

I do not think we ever intended on making out $ pave over your $. So we will fix this.

Comment 7

[David Dahl :ddahl]

(In reply to comment #2)
> when I'm debugging pages of my own
> making (so far primarily what I've used Web Console for) I know and want to
> interact with the Javascript environment I've created.

Technically, you are. the $ variable is local to the sandbox that the console uses to execute code in. the window.$ is your variable. The scopes are mixed here, but you as a user are unaware of this. We just need to lazily figure out if your $ exists and call that instead. Thanks for the feedback!

Comment 8

[Rob Campbell [:rc] (:robcee)]

I think it's even easier than that. During the initialization of JSTerm, we can detect if there's a $ (or $$) defined in the page's window object, and if there is, not add our own.

Comment 9

[Kevin Dangoor]

I believe the rationale for not doing it on initialization was a timing concern (that window.$ might come into existence after JSTerm's $ function, for example)

Comment 10

[Rob Campbell [:rc] (:robcee)]

oh yes. Silly me.

Comment 11

[Johnathan Nightingale [:johnath]]

This is something we'd have triaged as a softblocker before yesterday's dev call said that softblockers no longer have blanket approval to land. So, marking in the whiteboard, leaving the blocking flag untouched, but a reviewed and safe (trusting y'all!) patch has pre-approval to land, a=me.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch with test included.

The approach is to simply remove the $() and $$() functions before execution, when they are in the window object. This allows us to avoid problems of eval()-ing the respective functions, avoids any security troubles we might have in the implementation.

Another benefit of the approach is that we can support things like passing objects to the page function called by the user. If we'd have to eval() a string, things get complicated / nasty.

Looking forward to your feedback. Thanks!

Comment 13

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 512893 [details] [diff] [review]
proposed patch

this looks like it'll work. not sure we need to restore the $ functions to their original state after each run. Why not set sandbox.$ = window.$ when sandbox.$ != window.$ ?

Comment 14

[Mihai Sucan [:msucan]]

(In reply to comment #13)
> Comment on attachment 512893 [details] [diff] [review]
> proposed patch
> 
> this looks like it'll work. not sure we need to restore the $ functions to
> their original state after each run. Why not set sandbox.$ = window.$ when
> sandbox.$ != window.$ ?

If we do sandbox.$ = window.$ we overwrite our implementation "forever" (for the lifetime of the web console in that tab). Which means that when the user loads a different page, or when the scripts in the page delete window.$ (or change it to something else) ... the evaluation of $() will still point to the old window.$, which is technically wrong.

Choosing to delete sandbox.$ allows the code path to evaluate the real window.$ at all times, irrespective of changes.

I put our sandbox.$ back after evaluation because later when the user tries to evaluate $ the window.$ property may no longer be available (maybe a script in the page deleted window.$ or a newly loaded page has no window.$ defined).

Shall I make any specific changes to the patch? If you want, I can do what you requested.

Comment 15

[Mihai Sucan [:msucan]]

(In reply to comment #14)
> (In reply to comment #13)
> > Comment on attachment 512893 [details] [diff] [review]
> > proposed patch
> > 
> > this looks like it'll work. not sure we need to restore the $ functions to
> > their original state after each run. Why not set sandbox.$ = window.$ when
> > sandbox.$ != window.$ ?
> 
> If we do sandbox.$ = window.$ we overwrite our implementation "forever" (for
> the lifetime of the web console in that tab). Which means that when the user
> loads a different page, or when the scripts in the page delete window.$ (or
> change it to something else) ... the evaluation of $() will still point to the
> old window.$, which is technically wrong.

Correction: if we always update sandbox.$ to point to window.$ when the latter exists, then when the latter changes, sure, sandbox.$ also changes. Still, the case when window.$ is deleted still stands: we would need to put back our sandbox.$ implementation.

Comment 16

[Rob Campbell [:rc] (:robcee)]

yeah, I was thinking about this a little more last night and I couldn't think of a better way to do it. We do definitely need to check on every execution in case window.$ changes between evals. Still F+

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 512893 [details] [diff] [review]
proposed patch

Thanks for the feedback+!

Asking for review from Dolske.

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 512893 [details] [diff] [review]
proposed patch

Please add a (brief) comment here noting that this is done to prefer the page's $/$$ over ours when it exits.

a+ assuming this passes Tryserver (please run if not already done).

Comment 19

[Mihai Sucan [:msucan]]

Attachment: [checked-in] updated patch

Updated the patch to include a comment, as requested.

Tryserver builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-aa3f42fd6c94

Tryserver results:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=aa3f42fd6c94

Thanks for the review+ and approval2.0+.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 513428 [details] [diff] [review]
[checked-in] updated patch

http://hg.mozilla.org/mozilla-central/rev/5872649c4e86