Closed Bug 614350 Opened 14 years ago Closed 14 years ago

Web console's console object colliding with content breaks sites

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: sheppy, Assigned: rcampbell)

References

Details

Attachments

(2 files)

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
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.
Interesting -- the site works fine in Firefox 3.6, so this is a regression (of sorts) in Firefox 4.
MindTouch filed a bug with the vendor of their bug tracking system: http://youtrack.jetbrains.net/issue/JT-7553
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.
Blocks: devtools4
Priority: -- → P1
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.
(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.
…or we could just do all that work in this bug. ;)
(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()
(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. :)
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attached patch stubs (deleted) — Splinter Review
Attachment #493280 - Flags: review?(gavin.sharp)
requesting blocking as this could cause other sites to break.
blocking2.0: --- → ?
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?
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.
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?
> 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?
Attached patch Alternate patch (deleted) — Splinter Review
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
Attachment #493495 - Flags: review?(gavin.sharp)
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.
Attachment #493280 - Flags: review?(gavin.sharp)
Attachment #493495 - Flags: feedback+
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.
blocking2.0: ? → betaN+
(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?
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?
... 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?
(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 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)
Attachment #493495 - Flags: review?(gavin.sharp) → review+
Whiteboard: [needs-additional-test]
Assignee: rcampbell → ddahl
I can likely do this pretty quickly if you've got other stuff to work on, David. Let me know.
(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.
yeah, was reminded when Kevin changed the assignment. I'll get a patch in this weekend.
Assignee: ddahl → rcampbell
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs-additional-test]
test fixed in 2a75c2355ae9
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: