Closed Bug 352742 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect, major)

1.8 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: jruderman, Assigned: crowderbt)

References

(Blocks 1 open bug)

Details

(Keywords: testcase, verified1.8.1.1)

Attachments

(5 files, 1 obsolete file)

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".
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.
Flags: blocking1.8.1?
Target Milestone: --- → mozilla1.8rc1
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.
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
Attachment #238576 - Flags: review?(shaver)
minusing as per brendan's statement that this isn't really a blocker, but instead a silent, uncatchable failure. 
Flags: blocking1.8.1? → blocking1.8.1-
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
(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.
(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
> 

*** Bug 352986 has been marked as a duplicate of this bug. ***
The second patch has merge conflicts now :(
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.
Status: NEW → ASSIGNED
Attached patch roll-up and unbitrot (obsolete) — Splinter Review
This rolls up the last two, and resolves the merge issue Jesse had.  diff -w version coming next.
Assignee: general → crowder
Attached patch woops, bad diffSplinter Review
disregard last diff.  This is NOT the -w version.  That really WILL be next.
Attachment #239326 - Attachment is obsolete: true
Attached patch rollup with -wSplinter Review
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.
The combined (rollup) patch doesn't fix that either.
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.
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?
Sure.  Split out into bug 354246.
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 on attachment 239327 [details] [diff] [review]
woops, bad diff

Mind taking a look, mrbkap?
Attachment #239327 - Flags: review?(mrbkap)
Attachment #239327 - Flags: review?(mrbkap) → review+
Whiteboard: [needs checkin]
Whiteboard: [needs checkin] → [checkin needed]
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
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Whiteboard: [checkin needed]
Target Milestone: mozilla1.8rc1 → mozilla1.9alpha
Great, thanks gavin.  Jesse: if this resolves the original issue can you close this bug out?
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.
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: js1.7src
Flags: blocking1.8.1.1?
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
Flags: in-testsuite+
verified fixed 1.9 20061012 windows/linux
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment on attachment 239327 [details] [diff] [review]
woops, bad diff

Applies cleanly for me on branch
Attachment #239327 - Flags: approval1.8.1.1?
Comment on attachment 239327 [details] [diff] [review]
woops, bad diff

Approved for 1.8.1 branch, a=jay for drivers.  Please land asap.  Thanks!
Attachment #239327 - Flags: approval1.8.1.1? → approval1.8.1.1+
mozilla/js/src/jsobj.c 	3.208.2.41
mozilla/js/src/jsscan.c 	3.81.2.26
Keywords: fixed1.8.1.1
Target Milestone: mozilla1.9alpha → mozilla1.8.1
Version: Trunk → 1.8 Branch
verified fixed 20061123 1.8.1.1 windows/linux/mac*, 1.9 windows/linux
Attachment #238576 - Flags: review?(shaver)
Attachment #238579 - Flags: review?(shaver)
No longer blocks: 349611
Blocks: 349611
You need to log in before you can comment on or make changes to this bug.