Closed Bug 1178653 Opened 4 years ago Closed 4 years ago

Intermittent newTargetRectifier.js:4:9 Error: Assertion failed: got (void 0), expected function Inner


(Core :: JavaScript Engine: JIT, defect)

Not set



Tracking Status
firefox41 --- unaffected
firefox42 --- fixed
firefox43 --- fixed
firefox-esr38 --- unaffected


(Reporter: Waldo, Assigned: Waldo)


(Keywords: intermittent-failure)


(1 file, 1 obsolete file)

First appearance:

That in a push following up on:

Neither set of pushes does anything dealing with or the JITs.  This particular failure has, right now, appeared on only a single cgc build of eight thus completed -- or one of four, if you think this has anything to do with Windows specifically (which I do not).

Eric, you have any idea what might be happening here?
Flags: needinfo?(efaustbmo)
Summary: tests\basic\newTargetRectifier.js:4:9 Error: Assertion failed: got (void 0), expected function Inner → Intermittent newTargetRectifier.js:4:9 Error: Assertion failed: got (void 0), expected function Inner
efaust, I was running jit-tests with various GC zeal values and I noticed I can repro this one reliably on OS X with:

  JS_GC_ZEAL=14,5 dist/bin/js --ion-eager --no-threads ../jit-test/tests/basic/newTargetRectifier.js

Maybe this also works on Linux so you can use gdb/rr?

If it doesn't work for you I can look into it; it will probably be easier to track this down on OS X than on Windows.
Armed with Jan's find of a repro case on Linux, and rr, I was able to track down the problem.

It looks like jit::InvokeFunction() is just wrong for functions without jitcode. We just have to time the GC correctly, and here we go. is just too restrictive, because now there *is* a difference between Invoke and InvokeConstructor, and we cannot afford to get it wrong.

Changing that line to |if (constructing)| has silenced this error.

Unfortunately, that is not that savory, as it throws out the work we had just done computing a this value. I am pretty unhappy adding *another* InvokeConstructor() overload, but maybe that's best. Sigh.
Flags: needinfo?(efaustbmo)
Attached patch Fix (obsolete) — Splinter Review
OK, this should take care of both the bug and the concerns above. Allow InvokeConstructor() to take an argv with a |this|, so that we don't lose our work.
Assignee: nobody → efaustbmo
Attachment #8635540 - Flags: review?(jwalden+bmo)
Attached patch Counter-patchSplinter Review
So I got to the point of mostly grokking the current patch, but it was just too gnarly to really be happy about.  I wanted to see just how bad it would be to fix everything to a consistent inteface, with this one weird place doing something entirely different.  It turns out to be not horribly bad, with an interface that's worlds better.

A few notes on points of interest here.

InitArgsFromArrayLike could be split a little to have more common code, but it's not worth the time for negligible codesize win.

The spec's Construct(F, argumentList, newTarget) methods asserts that F and newTarget are constructors.  This patch, consistent with that, pushes IsConstructor checks into the callers.  Ion requires that the target (callee) in a |new| have a fully-isConstructor() typeset -- see IonBuilder::getPolyCallTargets -- so Ion's good.  Baseline delegates to stubs that call DoCallFallback (which I changed) and DoCallFallback (which delegates to SpreadCallOperation which I changed).  And I added checks to the interpreter.

Having both JS::Construct and js::Construct isn't ideal.  We should unify them, but probably a followup at this point.

Creating a js::Call taking a similar approach to what js::Construct does, consistent with the spec, seems like a really good idea.  Another followup bug.

Getting rid of JS::Construct functions, JS::New, etc. and simplifying to one or *maybe* two overloads seems like another followup bug.

Just outside array_of patch context is the IsConstructor verification that's conspicuously missing from the changes in the patch.

Wrapper::isConstructible() is totally wrong.  Is there a bug on file?  Need to cite in the test comment.  (Given we seem to be adding IsConstructor() calls everywhere these days, and they're just very wrong in those sorts of instances.  :-\ )

It's pretty dodgy that callable indirect proxies let you put a non-constructor in the constructor slot.  I delayed the error on that to construct-time out of a desire not to break too much code too early, but I could be persuaded to put it in Proxy.createFunction as well.  But really who cares.
Attachment #8638883 - Flags: review?(efaustbmo)
Assignee: efaustbmo → jwalden+bmo
Attachment #8635540 - Attachment is obsolete: true
Attachment #8635540 - Flags: review?(jwalden+bmo)
Comment on attachment 8638883 [details] [diff] [review]

Review of attachment 8638883 [details] [diff] [review]:

Apart from not commoning up some of the code that replaces InvokeConstructor() calls, I can't say as I can really claim my way is better. r=me.

::: js/src/jit/BaselineIC.cpp
@@ +9841,5 @@
>          return false;
>      }
>      if (op == JSOP_NEW) {
> +        // Callees from the stack could have any old non-constructor callee.

Do we have to hoist this code into all the callers? I would feel better if the "vm" kept control of how these arg structs were filled in. I know you and I disagree on this.

::: js/src/proxy/DirectProxyHandler.cpp
@@ +85,2 @@
>      RootedValue target(cx, proxy->as<ProxyObject>().private_());
> +    if (!IsConstructor(target)) {

Like, as aI mentioned above, this is like the 4th copy of exactly this code.
Attachment #8638883 - Flags: review?(efaustbmo) → review+
I added

template <class Args, class Arraylike>
inline bool
FillArgumentsFromArraylike(JSContext* cx, Args& args, const Arraylike& arraylike)
    uint32_t len = arraylike.length();
    if (!args.init(len))
        return false;

    for (uint32_t i = 0; i < len; i++)

    return true;

to reduce some of that copying, then landed that.
I added a missing IsConstructor test to the VMFunctions.cpp path into construction code, and I tweaked one existing test a little bit to not pigeonhole the error messsage quite so much, then landed with that.  Try-results for those changes basically checked out, so I think this should be good now.
Flags: needinfo?(jwalden+bmo)
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8638883 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: observable since bug 1141865, tho the underlying issue existed before then
[User impact if declined]: a few oranges a day on aurora; a new JS feature, syntactically incompatible with existing JS, having buggy behavior if called in perf-sensitive code
[Describe test coverage new/current, TreeHerder]: one existing jstest, that only intermittently failed pre-fix -- no 100%-reliable testcase landed
[Risks and why]: see below
[String/UUID change made/needed]: N/A

Try-run of an aurora patch combining both landings here:

If I don't end up landing this, is the revision to land.

Gonna be honest, I don't see the pressing need to take this on stable branches.

The consequence of this bug is that, in certain cases, JIT code will treat |new Foo(...)| as if it were instead calling |Foo| with the provided arguments, providing an otherwise-empty object having |Foo.prototype| as its [[Prototype]] as the |this|.  This difference in behavior is invisible *except* if you use the fresh-in-ES6 || syntax -- syntax there's little reason to use outside of subclassing (which we don't support yet), and which will harmlessly evaluate to a safe but wrong value if used in a way that this bug bites.  In short, nobody is currently affected by this bug but our tests, and the issue isn't a security issue.

The patch's size is another reason I'd leave well enough alone.  The changes are fairly mechanical, the single *important* change is now clearly correct, and everything I changed is now clearer and simpler.  But it's still a decent chunk of change.

The only benefit is making sheriffs' lives easier eliminating one intermittent orange.  That's not nothing.  But if TBPL robot comments accurately track instances, we have 18 instances over about seven weeks' time: a hit every three days or so, even amidst m-c/inbound's high traffic levels.  (I'm assuming that rate of incidence is accurate; sheriffs can correct me if it's not.)  I wouldn't have expected that rate of incidence to merit uplift, especially with aurora's greatly-reduced traffic levels.

That said, this patch is well-understood enough I'm not going to formally *object* to uplifting.  It just seems dubiously valuable to me to take it.
Flags: needinfo?(jwalden+bmo)
Attachment #8638883 - Flags: approval-mozilla-aurora?
Ryan, you confirm that you want this patch in aurora? Thanks
Flags: needinfo?(ryanvm)
I'd prefer to take fix if the risk is manageable for this point in the cycle. Obviously I'll defer to you/Waldo on it.
Flags: needinfo?(ryanvm)
Comment on attachment 8638883 [details] [diff] [review]

OK, we are short on sheriffs right now, I want them^Whim to be happy and have an easier life. Moreover, this is the beginning of the 42 cycle and the potential regressions should not be to hard to find, taking it.
Attachment #8638883 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.