1401014 - JIT reorders assignment statement, causing logic errors

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Marcus Cavanaugh [:mcav]]

Attachment: debugger-screenshot.png

In [1], Firefox's JIT appears to reorder statements in a way that causes logic errors.


Simplified Example
------------------

The error occurs in a function structured like this:

function f(obj) {
    var x = obj.x; // obj.x is non-null
    var y = obj.y;
    var z = obj.y = obj.x; // ERROR: obj.x is null here.
    obj.x = null;
}

(This is a simplified example, not a test case.) In this example, `z` and `x` should be the same non-null value. But with the JIT enabled, `obj.x = null` appears to be swapped with the previous line, assigning `null` to `obj.x` before the previous statement has executed.


In-Vitro Reproduction
---------------------

A screenshot of the debugger (attached) shows the logic error in practice, on real code. Per the annotations, vars `window.a` and `b` should be identical, but they are not. (Please excuse the noisiness of the screenshot.)

This issue surfaced within a JS library, MobX, and the thread tracking their descriptions can be found here[1]. This error has been reproduced by several different people, as noted in the thread.


Test Case
---------

Unfortunately, we have been unable to isolate the issue to a minimal test case. I've described repro steps in [1] that involve downloading a project and manually triggering the case, but reproducibility seems to depend on the optimizations being triggered in the JS engine.

A commenter has noted that this bug does not occur when `javascript.options.baselinejit` is disabled.

Perhaps someone on the JIT team would have insight as to how to build a test case that invokes the JIT deterministically (or simply insight as to what might cause this reordering).

[1]: https://github.com/mobxjs/mobx/issues/614#issuecomment-264565920

Comment 1

[Ted Campbell [:tcampbell]]

I'm unable to build the sensor web branch. Are you able to email me a tarball of the build output when it fails for you?

Comment 2

[Ted Campbell [:tcampbell]]

Managed to reproduce now. Disabling baseline makes it go away.

Comment 3

[mweststrate]

Would rewriting the nested assignment as 

```
var z = obj.x // (or y)
obj.y = z
```

Prevent the issue?

Comment 4

[Matjaz Gregoric]

(In reply to mweststrate from comment #3)
> Would rewriting the nested assignment as 
> 
> ```
> var z = obj.x // (or y)
> obj.y = z
> ```
> 
> Prevent the issue?

No, I tried that but it did not help.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Note that disabling baseline disables Ion as well. Asmjs and WASM should be unaffected, but those probably aren't relevant here, so it effectively disables the JITs altogether. It might be interesting to try with javascript.options.ion set to false to see if the baseline compiler is implicated.

Comment 6

[Jan de Mooij [:jandem]]

Thanks for the bug report. Let's get this fixed in 57.

Comment 7

[Ted Campbell [:tcampbell]]

Regression range was a busy day for the JIT... https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=16663eb3dcfa759f25b5e27b101bc79270c156f2&tochange=829d3be6ba648b838ee1953fdfa1a477dace752f

The javascript.options.ion flag fixes, so it is likely an Ion problem. I'm trying to reduce and capture a reduced case.

Comment 8

[Ted Campbell [:tcampbell]]

Consider the following simplification:

> function f(obj) {
>     var z = obj.x;
>     obj.x = null;
>     bailout();
>     z.foo();
> }

If |IonBuilder::setPropTryInlineAccess| is used to emit |obj.x = null|, a resume point does not get generated. As a result, when bailout-to-Baseline occurs (in the real case, due to a rangecheck) the execution will resume from the start of function f. This requires z to reload obj.x which has since been clobbered.

I'll add the missing resume point and hopefully this doesn't regress any perf.

Comment 9

[mweststrate]

Ignorant and just curious question (feel free to ignore). Do I understand it correctly that this relates to error handling / recovery (given the bailout?). Cause not everybody using the library with FF seems to be running into the original issue. If it relates to error handling that would explain why.

Comment 10

[Ted Campbell [:tcampbell]]

The term "bailout" inside SpiderMonkey refers de-optimizing the JIT code to something more general case after detecting tricky cases (like exceptions). In this sensorweb case the observing array needed to perform an allocation to resize (it was lazy before) triggering the de-optimization.

One reason that others may not experience is if they messing with the |derivation| object. We only hit this broken code path because we were able to do a lot of analysis about |derivation| and implement it in an efficient way. There are many reasons that would prevent us from optimization the memory layout of |derivation| and as such wouldn't hit this bug.

Comment 11

[Ted Campbell [:tcampbell]]

Attachment: Bug 1401014 - Fix resume point in IonBuilder::setPropTryInlineAccess

Review commit: https://reviewboard.mozilla.org/r/181610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181610/

Comment 12

[Ted Campbell [:tcampbell]]

Bug is that there is a missing resume point, resulting in statements re-executing even after their RHS has been clobbered. This patch adds the missing resume point. This issue goes back to FF49 at least.

We should land this for 57 and backport to ESR-52 since js frameworks hit this in the wild.

(Sorry, the test case is pretty terrible :(

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8910120 [details]
Bug 1401014 - Fix resume point in IonBuilder::setPropTryInlineAccess

https://reviewboard.mozilla.org/r/181610/#review187036

Good catch, thanks for fixing.

I wonder if we should MOZ_ASSERT_IF(ins->isEffectful(), ins->resumePoint()) somewhere (there might be some false positives we have to whitelist). Please file a follow-up bug if you agree.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Ted Campbell [:tcampbell] from comment #12)
> We should land this for 57 and backport to ESR-52 since js frameworks hit
> this in the wild.

Note, IonBuilder.cpp changed to use MOZ_TRY in 53, you will have to make a special patch for esr52.
See https://hg.mozilla.org/mozilla-central/rev/0eca45c6fb2d67b318aed4484370d7a3477a934a

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bd12fd8c15e
Fix resume point in IonBuilder::setPropTryInlineAccess r=jandem

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1bd12fd8c15e

Comment 17

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.