614350 - Web console's console object colliding with content breaks sites

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Eric Shepherd [:sheppy]]

For example, visit http://youtrack.developer.mindtouch.com/issue/MT-7969 -- a bug in the MindTouch bug tracker. The content of the comments box fails to load. If you look in the console log you'll see:

15:10:20.235: Exception: console.time is not a function Source File: http://youtrack.developer.mindtouch.com/_resourceBundle/bundledJavaScript-5f0a6b2d0e6b0a41066f0547357b1624.js Line: 7833, Column: 0 Category: content javascript

Comment 1

[David Dahl :ddahl]

Sounds like a mindtouch js bug as console.time is webkit only. 

Resolve WONTFIX - or file a bug for implementation of console.time - which won't happen right away.

Comment 2

[Eric Shepherd [:sheppy]]

Interesting -- the site works fine in Firefox 3.6, so this is a regression (of sorts) in Firefox 4.

Comment 3

[Eric Shepherd [:sheppy]]

MindTouch filed a bug with the vendor of their bug tracking system: http://youtrack.jetbrains.net/issue/JT-7553

Comment 4

[Kevin Dangoor]

Lets use this bug to track any instances of this kind of behavior we come across and figure out what (if anything) to do about it. This *is* a bug in the website (given that console is only a de facto standard, there's not much a site can count on beyond console.log if it finds a console object).

Also: I'll note that Firebug has console.time as well.

Comment 5

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

I'm wondering if it makes sense to create stubs for the missing API functions in our console object so errors like these don't crop elsewhere. There are bound to be some sites in the wild with these types of debugging calls in production.

Comment 6

[David Dahl :ddahl]

(In reply to comment #5)
> I'm wondering if it makes sense to create stubs for the missing API functions
> in our console object so errors like these don't crop elsewhere. There are
> bound to be some sites in the wild with these types of debugging calls in
> production.

Indeed, I think that we should open a bug on each one, both for a stub that just prints a warning to the console about using a not-yet-supported api call, and for the implementation of each.

Comment 7

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

…or we could just do all that work in this bug. ;)

Comment 8

[David Dahl :ddahl]

(In reply to comment #7)
> …or we could just do all that work in this bug. ;)

Maybe the stub functions, but the real implementations will require a bug each:

console.time()
console.timeEnd()
console.exception()
console.assert()
console.clear()
console.dir()
console.dirxml()
console.trace()
console.group()
console.groupCollapsed()
console.groupEnd()
console.profile()
console.profileEnd()
console.count()
console.table()

Comment 9

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

(In reply to comment #8)
> (In reply to comment #7)
> > …or we could just do all that work in this bug. ;)
> 
> Maybe the stub functions, but the real implementations will require a bug each:

yes, certainly.

Firebug's Console API documentation is here:

http://getfirebug.com/wiki/index.php/Console_API

but it looks like you captured their functions. :)

Comment 10

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

Attachment: stubs

Comment 11

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

requesting blocking as this could cause other sites to break.

Comment 12

[Philip Chee]

Hrm. Won't implementing stubs cause sites to break in *other* ways? e.g. a javascript framework does feature sniffing and notices that console.foo is available so tries to use that? Seems to me that the bug is in the Mindtouch code rather than us since they should be doing proper feature sniffing and not (broken) browser sniffing. Perhaps jresig can give us some insights?

Comment 13

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

I suppose that's possible, though I expect the failure is going to be less damaging than it will be for a site that doesn't bother checking for the feature and just fires off a console.time() function as Mindtouch's does, expecting a result.

It's a good point. Interested to hear John's and my reviewer's comments.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I would really not pollute our console object with stubs for sites that assume Gecko+window.console == firebug, unless we're really sure that the problem is widespread. Do we have any indication that it affects more than that Mindtouch code?

Comment 15

[Philip Chee]

> doesn't bother checking for the
> feature and just fires off a console.time() function as Mindtouch's does,
> expecting a result.

Any cross platform Javacript framework or library (Prototype, JQuery, etc) that blindly fires off console.foo() without checking for the feature is likely to be broken in multiple other ways as well.

WONTFIX/NOTOURBUG => MindTouch/Tech Evang?

Comment 16

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Alternate patch

I hit this on my local government's tax payment page[1] and a quick look through Google code search[2] reveals a fair amount of code that would also be impacted by this. The assumption that the console object == Firebug/WebKit/others has been a fair one for authors to make for years. It may no longer be (or ever was) a valid one, but declaring them broken and attempting to evangelize is neither a battle worth fighting nor one that makes much sense given that this is not standardized, it's not the first implementation to exist on the web and the failure mode is fatal.

This patch simply makes calling any unknown methods on the console object do nothing for authors who're doing it wrong and makes them undetectable for authors doing it right.

[1] http://pastebin.mozilla.org/867326
[2] http://www.google.com/codesearch?hl=en&q=typeof\(console+lang%3Ajavascript

Comment 17

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

I considered doing it your way too, Ryan, but figured I'd err on the side of API compatibility. Considering there are some methods in those stubs we probably wouldn't implement (e.g., group/groupEnd?), I think I'm in favor of your patch.

Gavin, I think it's likely that there are sites out there that will be broken. Maybe a good number of them. It's hard to know how many sites do a simple lookup for window.console and then do things based on that result. It's wrong, but it's probably going to be harder to get them to do the right thing right away.

Then again, it's possible their sites will break anyway when they try to do something with console.time and have a null result.

Getting some Tech Evangelism on this would probably be a good thing too. Maybe a blog post or two is in order to get it started.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

I guess noSuchMethod is probably the way to go. A basic test should probably be added to test_consoleAPI.html; I guess it doesn't hurt to also include the browser chrome test.

Perhaps we should make __noSuchMethod__ print a warning about the method not actually being implemented? Would require a new string I guess, so maybe something to do post-branching.

Comment 19

[Kevin Dangoor]

(In reply to comment #18)
> Perhaps we should make __noSuchMethod__ print a warning about the method not
> actually being implemented? Would require a new string I guess, so maybe
> something to do post-branching.

Yeah, that makes sense.

So, to be clear on the status here we need another test added to Ryan's patch and then this one's good to go?

Comment 20

[Kevin Dangoor]

cc'ing l10n. Johnathan had a good point that while the ideal here might be to have a localized string, programmers working with JavaScript are somewhat accustomed to dealing with English error messages.

Perhaps we add an English error message for now, which is certainly friendlier than just silently ignoring the calls and add a localized form later on?

Comment 21

[Axel Hecht [:Pike]]

... and CCing :rtl.

I'd go for a plain english string for now, yeah.

For a localized string, I'd be curious how RTL would handle a LTR method name in a LTR console with a RTL error description?

Comment 22

[(no longer active)]

(In reply to comment #21)
> For a localized string, I'd be curious how RTL would handle a LTR method name
> in a LTR console with a RTL error description?

We currently make console input and output left-to-right in RTL mode.  So I guess given bidi resolution on top of it, we should be no worse than the case where you print the .textContent of a node containing RTL stuff...

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 493495 [details] [diff] [review]
Alternate patch

r=me with a simple test added to test_consoleAPI.html that ensures an unknown method doesn't throw. We can file a followup for the warning.

nit: remove CA_nsm's unused arguments.

(this has a context-only conflict with bug 612405, so might have to rebase if that ends up landing first)

Comment 25

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

I can likely do this pretty quickly if you've got other stuff to work on, David. Let me know.

Comment 26

[David Dahl :ddahl]

(In reply to comment #25)
> I can likely do this pretty quickly if you've got other stuff to work on,
> David. Let me know.

you are a scholar and a gentleman, feel free to take.

Comment 27

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

yeah, was reminded when Kevin changed the assignment. I'll get a patch in this weekend.

Comment 28

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

checked in: 

http://hg.mozilla.org/mozilla-central/rev/0b6a14a73d94

Comment 29

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

test fixed in 2a75c2355ae9