Closed Bug 1139794 Opened 5 years ago Closed 5 years ago

Remove __noSuchMethod__ from the console object

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

The console object currently uses __noSuchMethod__ to silently accept console.foo() etc. __noSuchMethod__ is a non-standard SpiderMonkey feature that we really want to remove. It also doesn't work reliably with the JITs.

Bug 629607 was filed to replace console's __noSuchMethod__ with a proxy, not sure why that bug was marked fixed because it didn't actually fix this.

Anyway, do we really need this silently-accept-anything behavior? I checked Chrome and Safari and they don't do this either. It seems like we could get a list of 'console' methods in Chrome, Safari and IE, and simply add no-ops for those or something. Would that be acceptable? What am I missing?

To me it also seems confusing to silently ignore calls. For instance, if I use console.warning("Foo") nothing shows up because I need console.warn instead. In this case, throwing a good old TypeError seems preferable.

Using a proxy could also work, but proxies will likely slow down all calls and I don't know if that's really worth it for this feature.

I'd be happy to work on this bug, once we decide how we're going to fix it.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Jan de Mooij [:jandem] from comment #0)
> It seems like we could get
> a list of 'console' methods in Chrome, Safari and IE, and simply add no-ops
> for those or something.

Also, if we do this we can maybe show a nice warning: "Sorry, we don't support console.foo() (yet)!", which seems more useful than completely ignoring the call.
Flags: needinfo?(amarchesini)
I agree with you, and I'm happy to review your patches if you want to work on this bug.
BTW, we really need to standardize the console API.
Flags: needinfo?(amarchesini)
FWIW, in Chrome 42 I get:

> Object.getOwnPropertyNames(console.__proto__.__proto__)
["debug", "error", "info", "log", "warn", "dir", "dirxml", "table", "trace", "assert", "count", "markTimeline", "profile", "profileEnd", "time", "timeEnd", "timeStamp", "timeline", "timelineEnd", "group", "groupCollapsed", "groupEnd", "clear", "constructor"]

In Nightly:

> Object.getOwnPropertyNames(console.__proto__).toSource()
["log", "info", "warn", "error", "exception", "debug", "table", "trace", "dir", "group", "groupCollapsed", "groupEnd", "time", "timeEnd", "profile", "profileEnd", "assert", "count", "__noSuchMethod__", "constructor"]

Properties in Chrome but not in Nightly:

["dirxml", "markTimeline", "timeStamp", "timeline", "timelineEnd", "clear"]

console.clear() clears the console, that seems pretty useful. Maybe we should add that one too?

The others all return undefined and markTimeline, timeline and timelineEnd print a deprecation warning.
(In reply to Jan de Mooij [:jandem] from comment #3)
> console.clear() clears the console, that seems pretty useful. Maybe we
> should add that one too?

I found bug 659625, nevermind.
In Safari 8 but not in Nightly: ["clear", "dirxml", "timeStamp"]

So that's just a subset of the extra methods Chrome has. Will check IE later today.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
console.timeStamp is bug 922221. console.dirxml is bug 922212. console.timeline/timelineEnd were discussed in bug 1050502.

See also the following doc which is the best and most recent attempt at standardizing this that I know of:

https://github.com/DeveloperToolsWG/console-object/blob/master/api.md
Attached patch WIP (obsolete) — Splinter Review
This patch adds the no-op methods mentioned before.
Attached patch PatchSplinter Review
This works and fixes the test failures I saw on Try.

There are some similar uses of __noSuchMethod__ related to the console object in the addon-sdk, but I can fix these later.
Attachment #8573239 - Attachment is obsolete: true
Attachment #8573320 - Flags: review?(amarchesini)
Comment on attachment 8573320 [details] [diff] [review]
Patch

Review of attachment 8573320 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me!
Attachment #8573320 - Flags: review?(amarchesini) → review+
This was added so that console methods that other browsers support but we don't yet don't break functionality when testing quickly in firefox. Additionally, other browsers have indicated they plan on implementing this as well. Saying it is nonstandard paints it in a worse light than it really is because the whole console object is nonstandard.

https://github.com/DeveloperToolsWG/console-object#ideas

I think we should maintain the never-throw behavior, but it doesn't matter to me whether it is implemented with __noSuchMethod__ or with a proxy. A warning message would be a nice addition, but it isn't an either or situation.
(In reply to Nick Fitzgerald [:fitzgen] from comment #10)
> Additionally, other browsers have indicated they plan on implementing this
> as well.

Do you know if there are any WebKit/Blink/Chromium bugs or mailing list threads for this?

> I think we should maintain the never-throw behavior, but it doesn't matter
> to me whether it is implemented with __noSuchMethod__ or with a proxy.

Unfortunately it's not easy to do this because:

(1) __noSuchMethod__ is non-standard and buggy, and we really want to remove it from SpiderMonkey. It's a lot of complexity to keep in the engine.

(2) Proxies are much slower and more heavyweight. This may be fine for a feature everybody else supports, but considering no other browser (and Firebug) has this I'm not sure it's really all that useful/necessary.

Anyways, I won't land the patch in this bug if you're not OK with it, but I'm not sure how we can move forward without it.
Sounds like Chrome may not be doing it for a while: https://twitter.com/fitzgen/status/573578731534487552

I guess I'm ok with removing it, but I don't feel great about it.
If you find other browser vendors thinking about implementing this, I'm happy to argue any and all browser vendors into the ground to not have everyone implement this thoroughly-broken API.  People tried to add something like it to ES6, but the attempt failed -- because the ECAMScript meta-object protocol just doesn't work the way this API thinks it works.

Ultimately, __noSuchMethod__ in the console object is a horrible hackaround for the failure to standardize the console object.  When we preserve the hackaround, we actively hinder bringing the console object into the standardized, open web, instead requiring that everyone reverse-engineer what the major browsers have done.  Put bluntly, that violates the Mozilla mission.
Blocks: 1140324
Blocks: 1140361
https://hg.mozilla.org/mozilla-central/rev/95fe77826c1f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.