618574 - TM: "Assertion failure: !IsFunctionObject(v),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

eval("\
  (function(){\
    x=Proxy.createFunction((\
      function(){\
        return{\
          iterate:function(){\
            return(function(){})\
          }\
        }\
      }()\
    ),Object.getOwnPropertyDescriptor);\
    function a(z){\
      for(v in z)n\
    }\
    for each(let e in[\
      String,String,String,Number,Number,String,new String,new Number,x\
    ]){\
      a(e)\
    }\
  })\
")()

asserts js debug shell on TM changeset 1002cba2f2d6 with -j at Assertion failure: !IsFunctionObject(v),

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   47546:9c869e64ee26
user:        Luke Wagner
date:        Wed Jul 14 23:19:36 2010 -0700
summary:     Bug 549143 - fatvals

Comment 2

[Luke Wagner [:luke]]

fatvals added a slew of assertions, including this one.  If you put

diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
--- a/js/src/jstracer.cpp
+++ b/js/src/jstracer.cpp
@@ -2745,16 +2745,17 @@ TraceMonitor::mark(JSTracer* trc)
     switch (type) {
       case TT_OBJECT:
+        JS_ASSERT(!(*(JSObject**)slot)->isFunction());
         v = OBJECT_TO_JSVAL(*(JSObject**)slot);

To the cset before fatvals, the given testcase still asserts.

Gary, do you suppose you could bisect back a bit further using this added assertion?

Comment 3

[Jason Orendorff [:jorendorff]]

Simpler:

var x = Proxy.create({
    iterate: function () {
            return function () {};
        }
    });

for each (var e in [{}, {}, {}, {}, {}, {}, {}, {}, x])
    for (var v in e)
        ;

Stack is just js::LeaveTree -> js::NativeToValue -> assertion. It happens when leaving trace after a deep bail.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 5

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/47a4f03b5947

Comment 6

[Jesse Ruderman]

bhackett says this caused serious regressions including bug 619880.

Comment 7

[Gregor Wagner [:gwagner]]

The comparison on the webkit sunspider page doesn't work any more with this patch.

Comment 8

[Brian Hackett [Laid off!]]

Backed out:

http://hg.mozilla.org/tracemonkey/rev/123fa989ef70

Comment 9

[Nicholas Nethercote [inactive]]

When a new patch for this bug is written, can the author please check that this program from bug 619880 runs without asserting?  Thanks.

  for(let a in[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0])
    for(d in[]);

Comment 10

[Daniel Veditz [:dveditz]]

How far back does this bug go -- affects 1.9.x versions?

Is this a critical security bug or bogus assertion?

Comment 11

[Johnny Stenback (:jst)]

sg:critical? until proven otherwise.

Comment 12

[Luke Wagner [:luke]]

The original patch needs to "vp->setObject(*obj)" in ObjectToIterator before calling js_ValueToIterator.  With that comment0, comment 3, comment 9, and trace tests pass.

Comment 14

[Robert Sayre]

is this fixed-on-tm?

Jason Orendorff – Unbox iterator after deep bail. Bug 618574, r=dvander. Second landing, including a fix thanks to luke.

Comment 15

[David Mandelin [:dmandelin]]

(In reply to comment #14)
> is this fixed-on-tm?
> 
> Jason Orendorff – Unbox iterator after deep bail. Bug 618574, r=dvander. Second
> landing, including a fix thanks to luke.

Yes.

http://hg.mozilla.org/tracemonkey/rev/1f9f35be9840

Comment 16

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.