896535 - Promise: `then(console.log)` is not working as expected

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Matteo Ferretti [:zer0] [:matteo]]

Sorry for the Summary, feel free to change with something more appropriate.

The following code is working as expected, logging `42`:

    new Promise(r => r.resolve(42)).then(x => console.log(x));

But:

    new Promise(r => r.resolve(42)).then(console.log);

Doesn't log anything. Notice that `log` is already bound to `console`, but also this code:

    new Promise(r => r.resolve(42)).then(console.log.bind(console))

Gives the same result: 42 is not logged.

Am I missing something in the Promise specs?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Andrea, can you take a look?

Comment 2

[Andrea Marchesini [:baku]]

Ok, this is fun. The reason why it doesn't work is because console.log generates the stack trace here:

https://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js#401

but the callee of console.log() is C++ so stack array is empty. and

https://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPI.js#263

'frame' is undefined.

I know that there is a plan for changing the way we calculate the stack trace and this depends by bug 767938. I need a feedback from ConsoleAPI developers.

Comment 3

[Mihai Sucan [:msucan]]

If there's no JS frame in the stack being built by getStackTrace(), then please make a patch for _processQueuedItem() to accept such situations. This should be accepted - the code should not fail if, for some reason, we cannot determine the frames for a console API call. Thank you!

Comment 4

[Andrea Marchesini [:baku]]

Attachment: console.patch

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 783184 [details] [diff] [review]
console.patch

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

Thank you for the patch!

The Console API is used for web apps and we do not want to expose non-JavaScript frames in the stackframes list. What I suggested is we update _processQueuedItem() to not throw when the stackframes array is empty. Could we do that?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: console.patch

Good point. This is probably better.

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 784404 [details] [diff] [review]
console.patch

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

Patch looks good. Thanks for the quick update!

Can you please write a test?

::: dom/base/ConsoleAPI.js
@@ +266,5 @@
> +    } else {
> +      frame = {
> +        filename: '',
> +        lineNumber: 0,
> +        functionName: '[native code]',

Let's leave the function name empty. [native code] is usually displayed when you try toString() on functions.

nit: please use double quotes for consistency with the file.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: console.patch

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 784437 [details] [diff] [review]
console.patch

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

Patch look good. Thank you!

::: dom/tests/browser/browser_ConsoleAPITests.js
@@ +86,5 @@
> +    try {
> +      testNativeCallback(aSubject.wrappedJSObject);
> +    } catch (ex) {
> +      // XXX Exceptions in this function currently aren't reported, because of
> +      // some XPConnect weirdness, so report them manually

Do we have a bug about this?

@@ +100,5 @@
> +function testNativeCallback(aMessageObject) {
> +  is(aMessageObject.level, "log", "expected level received");
> +  is(aMessageObject.filename, "", "BBfilename matches");
> +  is(aMessageObject.lineNumber, 0, "BBlineNumber matches");
> +  is(aMessageObject.functionName, "", "BBfunctionName matches");

Why "BB" prefix for these messages?

::: dom/tests/browser/test-console-api.html
@@ +47,5 @@
>          console.groupEnd("b", "group");
>        }
> +
> +      function nativeCallback() {
> +        SpecialPowers.setBoolPref("dom.promise.enabled", true);

Do we need to reset the preference once the test ends?

Comment 10

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=aca46cfccdb7

Comment 11

[Andrea Marchesini [:baku]]

Attachment: console.patch

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f00ecb727d61

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f00ecb727d61