753120 - HostFunctionObject passes NULL as call parameter => Assert Failure

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Bug 733017 introduced the HostFunctionObject class.

I don't grok all the details yet, but there's a pretty clear bug in the constructor for HostFunctionObject (reported to me by jasowill):

  HostFunctionObject::HostFunctionObject(VTable* vtable, ScriptObject* delegate)
      : FunctionObject(vtable, NULL /* MethodEnv callee */) {
      setDelegate(delegate);
      m_call_ptr = (FunctionProc) &callHostFunction;
  }

in particular, it is passing NULL as the second argument to the FunctionObject constructor, which looks like this:

    FunctionObject::FunctionObject(VTable* cvtable, MethodEnv* call)
        : ClassClosure(cvtable, ClassClosure::createScriptObjectProc)
        , m_call_ptr(FunctionObject::callFunction)
        , m_callEnv(call)
    {
        AvmAssert(m_callEnv != NULL);
        // Since FunctionObject is (pseudo)final, we shouldn't need to calculate this every time,
        // but let's reality-check here just in case.
        AvmAssert(calcCreateInstanceProc(cvtable) == ClassClosure::createScriptObjectProc);
    }

Note that m_callEnv == call == NULL here.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

To unblock engineering in the short term, I removed the assertion from the constructor (bubbling it forward to precede the points that are actually making method calls on m_callEnv) in the FlashRuntime Main in CL 1061746.

This only landed in FlashRuntime Main, not our team (avm) branch.  We should:

1. Figure out what's going on here, and

2. Add at least one internal test of whatever functionality is provided by HostFunctionObject, so that we would catch absurdity like this in our own Debug builds.

Comment 2

[William Maddox]

In the new scheme, an AS3 function (FunctionObject) is invoked via m_call_ptr, which may point to a thunk.  In FunctionObject, m_call_ptr points to a static method FunctionObject::callFunction(), which needs the environment to perform the call like so: f->m_callEnv->coerceEnter(argc, argv).  In HostFunctionObject, m_call_ptr instead points to HostFunctionObject::callHostFunction(), which avoids the fastpath to coerceEnter(), and instead invokes f->call(), ignoring the environment.  Presumably, instances of HostFunctionObject are of derived classes that override virtual function FunctionObject::call(), as otherwise we'd get an infinite regress.

The high-order bit seems to be that m_callEnv is not needed to invoke a host function.  There are other methods such as FunctionObject::get_length(), however, that are public and inherited by HostFunctionObject (thus invokable from its instances) and that reference m_callEnv.

Comment 3

[Tamarin Bot]

changeset: 7393:7bee37ca3800
user:      Brent Baker <[e-mail]>
summary:   Bug 753120: Remove half-questionable assertion to unblock AIR submit tests. (p=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/7bee37ca3800

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #1)
> 2. Add at least one internal test of whatever functionality is provided by
> HostFunctionObject, so that we would catch absurdity like this in our own
> Debug builds.

(This still needs to be done; don't be fooled by comment 3.)