584587 - JM: execution stops in testcase using "with"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Version:
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/6d9d375a035f

Steps & results:
./js      --> PASS
./js -m   --> no output

(function() {
  var x;
  for (var i = 0; i < 2; ++i) {
    with({}) { 
      (function(){ x = 4; })();
    }
  }
  print("PASS");
})();

Comment 1

[Sean Stangl [:sstangl]]

Note: [jaeger] Abort    opcode "enterwith" not handled yet (/home/sstangl/foo.js line 3)

Comment 2

[Sean Stangl [:sstangl]]

The problem is that in jsinterp.cpp:4829, we have 'goto inline_return;' forced after a JaegerShot(). This assumes that after a call into MethodJIT-land, the next op is JSOP_STOP.

Since we don't yet compile 'enterwith', that assumption is broken.

The solution looks to be more complicated than just 'goto end_call;', since the results of the JaegerShot() have to be fed back to the interpreter (asserts trip).

Comment 3

[Sean Stangl [:sstangl]]

Nevermind, that's off-base. I'll poke it more later.

Comment 4

[Sean Stangl [:sstangl]]

This is a little interesting.

The (function(){x = 4; }) is JITted. It hits ic::BindName, tries to update(), calls generateStub(), and fails js_IsCacheableNonGlobalScope(scopeChain), returning NULL.

This NULL is then propagated all the way up through js_InternalThrow(), causing mjit::JaegerShot() to return false.

This then causes the interpreter to goto error, where it destroys the last remaining frame, unwinds the stack, and terminates execution.

Comment 5

[Sean Stangl [:sstangl]]

Attachment: Disregard BindName stub generation error.

The failing class is of type js_WithClass. This patch causes the failure to not cause a THROW() -- instead, the error is ignored. This is safe to do because existing inline and stub paths have not been modified, and are no less correct. The PIC is disabled, so the error cannot occur twice. I am unable to come up with a better solution if the object is indeed uncacheable.

I didn't include the test as part of the patch because it is difficult to assert that something didn't terminate early. It is possible I missed some trace-test feature enabling such an assertion.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 463477 [details] [diff] [review]
Disregard BindName stub generation error.

># HG changeset patch
># Parent acf031b46fd2bc17b9779854b6b41f7f762909d5
>[JAEGER] Disregard BindName stub generation error. b=584587.
>
>diff --git a/js/src/methodjit/PolyIC.cpp b/js/src/methodjit/PolyIC.cpp
>--- a/js/src/methodjit/PolyIC.cpp
>+++ b/js/src/methodjit/PolyIC.cpp
>@@ -1779,8 +1779,11 @@ class BindNameCompiler : public PICStubC
>             return obj;
>         }
> 
>-        if (!generateStub(obj))
>-            return NULL;
>+        /*
>+         * Disregard failure: If patching fails, then all previous paths
>+         * are still valid, so executing is not incorrect.
>+         */
>+        generateStub(obj);

Does this assume no THROW (no pending exception set, or reported OOM)? Isn't failure still possible here?

I don't see how this doesn't leave a reported error unpropagated in the OOM case but I haven't read generateStub. Just looking at context here, though, it isn't clear that JS_ReportOutOfMemory(f.cx) was not already called and failure propagated by a false return by a generateStub.

Either a method is fallible, or it isn't, absent any advice telling it to be a predicate (false return does not mean failure). Should generateStub take a "fail soft" flag?

>         return obj;
>     }

elsewhere in moo (at least one rev -- sorry if stale), I see code of this form:

   397         if (!pic.inlinePathPatched && !obj->scope()->branded())
   398             return patchInline(sprop);
   399         else
   400             return generateStub(sprop);
   401 
   402         return true;

Too many returns, and else after return is a non-sequitur -- avoid.

/be

Comment 7

[Sean Stangl [:sstangl]]

Attachment: Fix ic::BindNameCompiler::disable()

The problem can be solved in a better way: the definition of disable() was incorrect -- it is supposed to return true in this case, but instead returned NULL, which was silently coerced to false. So changing the definition fixes the issue. Verified by dvander.

Additionally, I fixed the update() redundancies that Brendan pointed out.

Comment 8

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/6355ebeaa681