1358939 - window object is serialized to huge string in errors by ValuetoSource

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Oriol Brufau [:Oriol]]

1. Go to about:newtab
2. Open the web console
3. Enter this code:
    var w = window.open(); w.close();
4. Enter this code:
    Reflect.apply(w);

Result:
    TypeError: can't access dead object

Expected result:
    TypeError: w is not a function

Both w() and Reflect.construct(w) produce the proper error.

Caused by bug 1303795.

Comment 1

[Oriol Brufau [:Oriol]]

This is so strange!

1. Go to about:newtab
2. Open the web console
3. Enter this code:
    var w = window.open(); w.close();
4. Enter this code:
    Reflect.apply(window);

Result:
    TypeError: can't access dead object

Expected result:
    TypeError: window is not a function

Yeah, it says window is a dead object!! It's like it thinks window points to w!

Reflect.construct, new and normal call seemed more well-behaved, but...

    var w = window.open(); w.close();

    Reflect.construct(window);               // TypeError: window is not a constructor
    Reflect.construct(new Proxy(window, {})) // TypeError: can't access dead object

    new window;                              // TypeError: window is not a constructor
    new new Proxy(window, {})()              // TypeError: can't access dead object

    window();                                // TypeError: window is not a function
    new Proxy(window, {})()                  // TypeError: can't access dead object

Comment 2

[Jeff Walden [:Waldo]]

I don't think this is a bug.

For starters, error messages aren't standardized in the spec, so we can make them whatever we want as long as the TypeError aspect remains correct.  Which it happens to, here.

But beyond that, the dead-object error message -- which doesn't appear in the context of bog-standard JS, but only in our weird host environment with its own spec-permissible peculiarities -- only appears as the byproduct of an error that occurs "between spec steps", as it were.  It's as if the spec algorithm listed steps 1-5, but we added a step 2.5 that tested for a dead object and threw an error (before the is-this-callable step, in effect).  Just in the same way that handling OOM is a host-specific process, by effectively injecting similar steps between well-specified spec algorithm steps.

Even assuming it's possible to make some smallish change to resolve this, it doesn't seem necessary to me, nor is it a valuable use of time.  I'd call this INVALID.

Comment 3

[Oriol Brufau [:Oriol]]

> error messages aren't standardized in the spec

Of course, but error messages should be useful and consistent.

Saying that a non-dead object is a dead object is definitely a bug.

Comment 4

[Oriol Brufau [:Oriol]]

Some problems mentioned in comment 1 were already fixed in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e6332dc0&tochange=5e464ee0 (bug 1322019? Does not seem much related).

Anyways, this still is wrong:

    var dead = window.open(); dead.close(); // undefined
    Reflect.apply(window);                  // TypeError: can't access dead object
    dead = 0;                               // 0
    Reflect.apply(window);                  // TypeError: [object Object]

The last line produces a longstring containing the serialization of an object (?) and this crashes the new console frontent (bug 1358937), so use the old console or a try..catch.

Reflect.apply(window) should not be affected by the value of the dead variable.

Comment 5

[Oriol Brufau [:Oriol]]

OK, I think I get what's happening.

The problem is that, when an error says that an object is not callable, it usually contains the identifier or the expression used to produce that code, e.g.

> [] instanceof [1,2,3];
  TypeError: [1, 2, 3] is not a function

However, for some reason, window does not appear as the string "window", the console attempts to serialize it instead.

> [] instanceof window;
  TypeError: ({close:function close() {
      [native code]
  }, stop:function stop() {
      [native code]
  }, focus:function focus() {
      [native code]
  }, blur:function blur() {
      [native code]
  }, open:function open() {
      [native code]
  }, alert:function alert() {
      [native code]
  }, confirm:function confirm() {
      [native code]
  }, prompt:function prompt() {
      [native code]
  }, print:function print() {
      [native code]
  }, showModalDialog:function showModalDialog() {
      [native code]
  }, postMessage:function postMessage() {
      [native code]
  }, captureEvents:function captureEvents() {
      [native code]
  }, releaseEvents:function releaseEvents() {
      [native code]
  }, getSelection:function getSelection() {
      [native code]
  }, getComputedStyle:function getComputedStyle() {
      [native code]
  }, matchMedia:function matchMedia() {
      [native code]
  }, moveTo:function moveTo() {
      [native code]
  }, moveBy:function moveBy() {
      [native code]
  }, resizeTo:function resizeTo() {
      [native code]
  }, resi…

The DeadObject problem was just that I declared a global variable which had a DeadObject value. The serialization of window attempted to access the DeadObject, and this threw a different error that shadowed the first one.

Until 2014-05-27 (cbe4f69c2e9c), the result was

    TypeError: invalid 'instanceof' operand ({})

Then something between 2014-05-27 and 2014-05-28 (https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbe4f69c2e9c&tochange=e017c15325ae), probably bug 789261, changed it to

    [Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"
     location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Then something between 2014-12-17 and 2014-12-18 (https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b7eb1ce0237d&tochange=0e441ff66c5e) changed it to

    [Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"
     location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Finally, something between 2015-01-24 and 2015-01-25 (https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c0f88b376e33&tochange=c18776175a69) changed it to behave as it does now.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

When I typed "[] instanceof window"  I didn't see a loooong console message, but I seemed encounter web console crash that regressed from bug 1304178. (Bug 1358937 is tracking the console frontend crash already.)

If I reverted to an old version before the regression, I got the error message |[Object Object] [Learn More]| where the "Learn More" redirects to the MDN page of |TypeError: "x" is not a function|. So, except the crash regression, I see no issues with the "huge string" symptom.

The latest release version, Firefox 53, on Win10 behaves as expected.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I don't think there's a DOM issue here, necessarily.

When SpiderMonkey throws an exception from "instanceof" due to the RHS not being a function, it tries to prettyprint the RHS.  So if you do:

  [] instanceof {a:5, b:"hey"}

the exception thrown will say:

  ({a:5, b:"hey"}) is not a function

Specifically, it will go through js::ReportValueErrorFlags, which will call DecompileValueGenerator() on the given value, which in this case does ValueToSource(), which for objects ends up calling .toSource(), which walks through all of the objects own properties and serializes their names and values.  And window has a _lot_ of own properties, per spec.

I don't see a way to affect the ValueToSource() behavior short of defining a toSource property on window, which would be nonstandard and web-observable, and we don't want to do that.

SpiderMonkey could provide us hooks for overriding this behavior if it wants to; then it might make sense to use them here.  But as things stand, there's nothing to do here on the DOM side.

Comment 8

[Oriol Brufau [:Oriol]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> When I typed "[] instanceof window"  I didn't see a loooong console message,

Sorry I didn't explain this very detailed, the long error crashes the new console, and the old one is not able to display it. To see the error, use

> try { Reflect.apply(window) } catch(e) { e.message }

or

> try { [] instanceof window } catch(e) { e.message }

Both codes seem to have the same problem, but in fact it's different (I guess because one it's an operator and the other a function).

I have managed to fix the former by modifying DecompileArgumentFromStack from

> if (op != JSOP_CALL && op != JSOP_CALL_IGNORES_RV && op != JSOP_NEW)
>     return true;

to

> if (op != JSOP_CALL && op != JSOP_CALL_IGNORES_RV && op != JSOP_NEW && op != JSOP_FUNAPPLY)
>     return true;

not sure if this can break things. And there is also JSOP_FUNCALL, not sure if it would make sense to include it too.


(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> Specifically, it will go through js::ReportValueErrorFlags, which will call
> DecompileValueGenerator() on the given value, which in this case does
> ValueToSource(), which for objects ends up calling .toSource()


Yes, but I don't understand why things like document don't have the same problem:

> [] instanceof document; // TypeError: invalid 'instanceof' operand document

document.toSource() is also a huge string, but it's not used. Why for window?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Yes, but I don't understand why things like document don't have the same problem:

Ah, interesting.  So what the JS engine actually does here is DecompileExpressionFromStack (which aims to get the actual variable name or expression used) and then fall back to ValueToSource if DecompileExpressionFromStack doesn't get something useful.

DecompileExpressionFromStack succeeds for "document" but not "window".  It fails for "window" because it calls FindStartPC with "v" set to a Value containing the Window.  But in the loop with this condition:

        } while (s != v || stackHits++ != skipStackHits);

it ends up with "s" being a WindowProxy for the Window.  So that equality doesn't hold and we end up falling back on ValueToSource.

"v" is a Window because proxies have a hasInstance hook and Wrapper::hasInstance forwards to the wrapped object, and WindowProxy is a Wrapper.

Anyway, we should probably fix FindStartPC to innerize "s"...

Comment 10

[Oriol Brufau [:Oriol]]

Seems a bit different now:

try { [] instanceof window } catch(e) { console.log(e.message) } // "window is not a function"
try { [] instanceof document } catch(e) {  console.log(e.message) } // "document is not a function"

try { Reflect.construct(window) } catch(e) {  console.log(e.message) } // "window is not a constructor"
try { Reflect.construct(document) } catch(e) {  console.log(e.message) } // "document is not a constructor"

try { Reflect.apply(window) } catch(e) {  console.log(e.message) } // "({close:function close() {\\n    [native code]\\n}, ...
try { Reflect.apply(document) } catch(e) {  console.log(e.message) } // "({get location() {\\n    [native code]\\n}, ...