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)
DevTools
General
Tracking
(blocking2.0 betaN+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: sheppy, Assigned: rcampbell)
References
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Gavin
:
review+
rcampbell
:
feedback+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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.
Reporter | ||
Comment 2•14 years ago
|
||
Interesting -- the site works fine in Firefox 3.6, so this is a regression (of sorts) in Firefox 4.
Reporter | ||
Comment 3•14 years ago
|
||
MindTouch filed a bug with the vendor of their bug tracking system: http://youtrack.jetbrains.net/issue/JT-7553
Comment 4•14 years ago
|
||
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
Updated•14 years ago
|
Priority: -- → P1
Assignee | ||
Comment 5•14 years ago
|
||
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•14 years ago
|
||
(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.
Assignee | ||
Comment 7•14 years ago
|
||
…or we could just do all that work in this bug. ;)
Comment 8•14 years ago
|
||
(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()
Assignee | ||
Comment 9•14 years ago
|
||
(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 | ||
Updated•14 years ago
|
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #493280 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 11•14 years ago
|
||
requesting blocking as this could cause other sites to break.
blocking2.0: --- → ?
Comment 12•14 years ago
|
||
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?
Assignee | ||
Comment 13•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
> 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•14 years ago
|
||
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)
Assignee | ||
Comment 17•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #493280 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Attachment #493495 -
Flags: feedback+
Comment 18•14 years ago
|
||
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.
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 19•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
... 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•14 years ago
|
||
(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•14 years ago
|
||
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+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs-additional-test]
Updated•14 years ago
|
Assignee: rcampbell → ddahl
Assignee | ||
Comment 25•14 years ago
|
||
I can likely do this pretty quickly if you've got other stuff to work on, David. Let me know.
Comment 26•14 years ago
|
||
(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.
Assignee | ||
Comment 27•14 years ago
|
||
yeah, was reminded when Kevin changed the assignment. I'll get a patch in this weekend.
Assignee: ddahl → rcampbell
Assignee | ||
Comment 28•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs-additional-test]
Assignee | ||
Comment 29•14 years ago
|
||
test fixed in 2a75c2355ae9
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•