352742 - Array.filter on object {valueOf: Function} halts JavaScript execution

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Steps to reproduce: paste this into the js shell.
  z = {valueOf: Function};
  print(2);
  try { [11].filter(z) } catch(e) { print(3); print(e); } print(4);

Result: 
  the last thing printed is "2".

Expected: 
  the last thing printed should be "4".

Comment 1

[Robert Sayre]

I tested a variation of this, with |z = {valueOf: Object};|. That runs to completion.

js_ValueToFunction(cx, vp, flags); is run on the predicate function.

When |z = {valueOf: Object};|, js_ValueToFunction returns null and sets cx->throwing = true.

When |z = {valueOf: Function};|, js_ValueToFunction returns null and sets cx->throwing = false.

Comment 2

[Robert Sayre]

2160 JSFunction *
2161 js_ValueToFunction(JSContext *cx, jsval *vp, uintN flags)
2162 {
2163     jsval v;
2164     JSObject *obj;
2165 
2166     v = *vp;
2167     obj = NULL;
2168     if (JSVAL_IS_OBJECT(v)) {
2169         obj = JSVAL_TO_OBJECT(v);
2170         if (obj && OBJ_GET_CLASS(cx, obj) != &js_FunctionClass) {
2171             if (!OBJ_DEFAULT_VALUE(cx, obj, JSTYPE_FUNCTION, &v))
2172                 return NULL;

                     ^-- we return here when | z = {valueOf: Function};|

2173             obj = VALUE_IS_FUNCTION(cx, v) ? JSVAL_TO_OBJECT(v) : NULL;
2174         }
2175     }
2176     if (!obj) {
2177         js_ReportIsNotFunction(cx, vp, flags);
2178         return NULL;

             when | z = {valueOf: Object};| we call js_ReportIsNotFunction,
             we are throwing, and we run to completion.
 
2179     }
2180     return (JSFunction *) JS_GetPrivate(cx, obj);
2181 }

I am not sure what the right fix is. Either we can make sure we are throwing in array_extra, or we can change this function... but I'm guessing what it does is right for most situations.

Comment 3

[Brendan Eich [:brendan]]

Attachment: Fix js_TryMethod to let errors be reported promptly

There's more to the story: js_ReportCompileErrorNumber (actually its static ReportCompileErrorNumber subroutine) does not report, not even to convert the error to an exception and throw to anyone who might be ready to catch it, if cx->errorReporter is null.

This goes back to mccabe's errors-as-exceptions work.  Shaver, do you remember why he did this?  I think we ought to just throw.

/be

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

minusing as per brendan's statement that this isn't really a blocker, but instead a silent, uncatchable failure. 

Comment 5

[Brendan Eich [:brendan]]

Attachment: Fix ReportCompileErrorNumber's bogus null test of cx->errorReporter

A diff -w is next.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Comment 7

[Brendan Eich [:brendan]]

I won't have time for this, but I'd be grateful if anyone who is a SpiderMonkey peer or member would take this bug, figure out if both of these patches are "more correct" (I think they both are), and land them.  Thanks,

/be

Comment 8

[Igor Bukanov]

(In reply to comment #3)
> Created an attachment (id=238576) [edit]
> Fix js_TryMethod to let errors be reported promptly

But why errors are ignored when accessing the property? I.e. the same silent failure still present when the method property is JS-getter that throws an exception.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #3)
> > Created an attachment (id=238576) [edit]
> > Fix js_TryMethod to let errors be reported promptly
> 
> But why errors are ignored when accessing the property? I.e. the same silent
> failure still present when the method property is JS-getter that throws an
> exception.

This is ancient code, older than ECMA-262, probably 11 years and four months old.  I've been leery of changing it and breaking something, but you are right that this is not ECMA behavior, neither is it consistent and helpful.  So for 1.9 it would be good to get rid of all exception and error-report suppression in js_TryMethod.

Someone please take this bug and carry it over the goal line.

/be
> 

Comment 10

[Robert Sayre]

*** Bug 352986 has been marked as a duplicate of this bug. ***

Comment 11

[Jesse Ruderman]

The second patch has merge conflicts now :(

Comment 12

[Brian Crowder]

Taking ownership of this bug and volunteering to unbitrot the patches as-needed.  Will post for review sometime tomorrow, probably.  In the meantime, Jesse is going to try applying them and running the fuzzer to see if they improve his world.

Comment 13

[Brian Crowder]

Attachment: roll-up and unbitrot

This rolls up the last two, and resolves the merge issue Jesse had.  diff -w version coming next.

Comment 14

[Brian Crowder]

Attachment: woops, bad diff

disregard last diff.  This is NOT the -w version.  That really WILL be next.

Comment 15

[Brian Crowder]

Attachment: rollup with -w

Comment 16

[Jesse Ruderman]

The first patch, by itself, does not fix this:

js> print(1); try{new Error({toString: function() { x.y } })}catch(e){} print(3);
1
typein:1: x is not defined

Let me know if that should be a separate bug.

Comment 17

[Jesse Ruderman]

The combined (rollup) patch doesn't fix that either.

Comment 18

[Jesse Ruderman]

I ran the fuzzer for a while with the roll-up patch and didn't find any new bugs.

I noticed one change that I think is good: several destructuring-decompilation bugs used to make ./js exit with code 0; now, some of these bugs make ./js print "out of memory" and exit with code 3 instead.

Comment 19

[Brian Crowder]

Jesse, I think the bug you demonstrate in comment #16 is a different bug.  I need to check the spec on this but I -think- (thanks to shaver for clarifying this for me) that the core problem is that were are suppressing exceptions anytime we are building an Exception object, without any regard to whether we are building one as an implicit response to an error or building one explicitly (not yet in an error-state).  Can you file as a new bug?

Comment 20

[Jesse Ruderman]

Sure.  Split out into bug 354246.

Comment 21

[Brian Crowder]

The other patch on this bug does seem to be an improvement.  Does it in fact resolve the first reported issue?  Anyone care to r+ it/check it in?  It looks pretty safe/harmless to me.

Comment 22

[Brian Crowder]

Comment on attachment 239327 [details] [diff] [review]
woops, bad diff

Mind taking a look, mrbkap?

Comment 23

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

I checked in attachment 239327 [details] [diff] [review] on the trunk.
mozilla/js/src/jsobj.c  	3.293
mozilla/js/src/jsscan.c 	3.115

Comment 24

[Brian Crowder]

Great, thanks gavin.  Jesse: if this resolves the original issue can you close this bug out?

Comment 25

[Jesse Ruderman]

Comment 0 works correctly now:

js> z = {valueOf: Function};
[object Object]
js> print(2);
2
js> try { [11].filter(z) } catch(e) { print(3); print(e); } print(4);
3
SyntaxError: missing ( before formal parameters
4

The syntax error makes sense -- it's the result of calling 
new Function("function").  ("function" is the hint passed to valueOf.)  And it's caught correctly.

Comment 26

[Jesse Ruderman]

The dup, bug 352986, works correctly now too:

js> j = ({toString: function() { eval("return"); }})
js> print(2); try { "" + j; } catch(e){print(e)} print(3);
2
SyntaxError: return not in function
3

Comment 27

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/js1_6/Array/regress-352742-01.js,v
done
Checking in regress-352742-01.js;
/cvsroot/mozilla/js/tests/js1_6/Array/regress-352742-01.js,v  <--  regress-352742-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_6/Array/regress-352742-02.js,v
done
Checking in regress-352742-02.js;
/cvsroot/mozilla/js/tests/js1_6/Array/regress-352742-02.js,v  <--  regress-352742-02.js
initial revision: 1.1
done

Comment 28

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061012 windows/linux

Comment 29

[Brian Crowder]

Comment on attachment 239327 [details] [diff] [review]
woops, bad diff

Applies cleanly for me on branch

Comment 30

[Jay Patel [:jay]]

Comment on attachment 239327 [details] [diff] [review]
woops, bad diff

Approved for 1.8.1 branch, a=jay for drivers.  Please land asap.  Thanks!

Comment 31

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

mozilla/js/src/jsobj.c 	3.208.2.41
mozilla/js/src/jsscan.c 	3.81.2.26

Comment 32

[Bob Clary [:bc] (inactive)]

verified fixed 20061123 1.8.1.1 windows/linux/mac*, 1.9 windows/linux