931304 - console API should toString() arguments synchronously

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "can't access dead object" {file: "resource://gre/components/ConsoleAPI.js" line: 369}]' when calling method: [nsITimerCallback::notify]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
************************************************************

I think console.log should toString its arguments synchronously.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes.  Asynchronous argument conversion is not compatible with WebIDL either.  Do we have a bug on file for WebIDLifying the console object?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Nope.

Furthermore, the async behavior here is somewhat on purpose, last I checked: it's a performance optimization since in common cases console stuff is never looked at....

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Hmm, that's going to be tricky ...

Comment 4

[Mihai Sucan [:msucan]]

We can add a Cu.isDeadObject() check and put a '[object DeadObject]' placeholder for the arguments that point to dead objects. Would this be fine?

Comment 5

[Jesse Ruderman]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Furthermore, the async behavior here is somewhat on purpose, last I checked:
> it's a performance optimization since in common cases console stuff is never
> looked at....

Is the performance benefit primarily from delaying the toString, or from delaying notifying the web console and browser console?

Comment 6

[Jesse Ruderman]

(In reply to Mihai Sucan [:msucan] from comment #4)
> We can add a Cu.isDeadObject() check and put a '[object DeadObject]'
> placeholder for the arguments that point to dead objects. Would this be fine?

Can a GC happen between the call to Cu.isDeadWrapper() and the call to toString?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Jesse Ruderman from comment #6)
> (In reply to Mihai Sucan [:msucan] from comment #4)
> > We can add a Cu.isDeadObject() check and put a '[object DeadObject]'
> > placeholder for the arguments that point to dead objects. Would this be fine?
> 
> Can a GC happen between the call to Cu.isDeadWrapper() and the call to
> toString?

Wrapper cutting isn't based on the GC.  It's based on deterministic points where we "know" a window should die.  Modulo existing violations of run to completion semantics like nested event loops the wrapper cannot get cut during script execution.

Comment 8

[Jesse Ruderman]

Oh, right.

console.log({toString: function() { window.close(); }});

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Jesse Ruderman from comment #5)
> (In reply to Boris Zbarsky [:bz] from comment #2)
> > Furthermore, the async behavior here is somewhat on purpose, last I checked:
> > it's a performance optimization since in common cases console stuff is never
> > looked at....
> 
> Is the performance benefit primarily from delaying the toString, or from
> delaying notifying the web console and browser console?

The primary reason was to delay notification of the web/browser console. We were blocking the content script execution.

Comment 10

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

Sounds like we should do the toString synchronously and see whether that negatively impacts the perf metrics we care about?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This testcase stopped reproducing in the pushlog range below:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58eca03214a6&tochange=8abc76dedec2

Calling this fixed by bug 965860.