1233331 - Crash [@ js::jit::IonScript::incrementInvalidationCount]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision 0babaa3edcf9 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager -D):

// jsfunfuzz-generated
x = 0;
try {
    a;
    b;
} catch (e) {}
// Adapted from randomly chosen test: js/src/jit-test/tests/gc/oomInDebugger.js
var g = newGlobal();
oomTest(function() {
    return Debugger(g);
});
// Adapted from randomly chosen test: js/src/tests/ecma_6/TemplateStrings/templLit.js
eval("function g() {}");

Backtrace:

#0  js::jit::IonScript::incrementInvalidationCount (this=<optimized out>) at js/src/jit/IonCode.h:542
#1  js::jit::Invalidate (types=..., fop=fop@entry=0x7fe230a4cae8, invalid=..., resetUses=resetUses@entry=true, cancelOffThread=cancelOffThread@entry=true) at js/src/jit/Ion.cpp:3068
#2  0x0000000000b3f467 in js::TypeZone::processPendingRecompiles (this=0x7fe22f78b630, fop=0x7fe230a4cae8, recompiles=...) at js/src/vm/TypeInference.cpp:2489
#3  0x00000000006ae60b in js::AutoEnterAnalysis::~AutoEnterAnalysis (this=0x7ffe1efeadd0, __in_chrg=<optimized out>) at js/src/vm/TypeInference-inl.h:309
#4  0x0000000000b51494 in js::AddTypePropertyId (cx=cx@entry=0x7fe230a18c00, group=0x7fe22f9602b0, obj=<optimized out>, id=..., type=...) at js/src/vm/TypeInference.cpp:2730
#5  0x0000000000b516d8 in js::AddTypePropertyId (cx=cx@entry=0x7fe230a18c00, group=<optimized out>, obj=obj@entry=0x7fe22f963060, id=..., id@entry=..., value=...) at js/src/vm/TypeInference.cpp:2773
#6  0x0000000000a83d2c in js::AddTypePropertyId (value=..., id=..., obj=0x7fe22f963060, cx=0x7fe230a18c00) at js/src/vm/TypeInference-inl.h:421
#7  js::NativeObject::setSlotWithType (overwriting=<optimized out>, value=..., shape=<optimized out>, cx=0x7fe230a18c00, this=<optimized out>) at js/src/vm/NativeObject-inl.h:286
#8  NativeSetExistingDataProperty (cx=cx@entry=0x7fe230a18c00, obj=..., obj@entry=..., shape=..., shape@entry=..., v=v@entry=..., result=..., receiver=...) at js/src/vm/NativeObject.cpp:2031
#9  0x0000000000a9b7b7 in SetExistingProperty (result=..., shape=..., pobj=..., receiver=..., v=..., id=..., obj=..., cx=0x7fe230a18c00) at js/src/vm/NativeObject.cpp:2255
#10 js::NativeSetProperty (cx=cx@entry=0x7fe230a18c00, obj=..., obj@entry=..., id=id@entry=..., value=..., receiver=..., qualified=qualified@entry=js::Qualified, result=...) at js/src/vm/NativeObject.cpp:2316
#11 0x00000000005e6a94 in js::SetProperty (result=..., receiver=..., v=..., id=..., obj=..., cx=0x7fe230a18c00) at js/src/vm/NativeObject.h:1485
#12 js::PutProperty (cx=0x7fe230a18c00, obj=..., id=..., v=..., strict=<optimized out>) at js/src/jsobj.h:935
#13 0x0000000000a9c2cc in js::DefFunOperation (cx=0x7fe230a18c00, script=..., scopeChain=..., funArg=...) at js/src/vm/Interpreter.cpp:4179
#14 0x00007fe23219ea39 in ?? ()
#15 0x00007ffe1efeb328 in ?? ()
#16 0x00007ffe1efeb2a8 in ?? ()
#17 0x00007ffe1efec780 in ?? ()
#18 0x0000000001b7a6a0 in InitPropGetterSetterInfo ()
#19 0x00007fe22f959880 in ?? ()
#20 0x00007fe231ffdafd in ?? ()
#21 0x0000000000000d01 in ?? ()
#22 0x00007fe22f9a0230 in ?? ()
#23 0x00007fe22f964070 in ?? ()
#24 0x00007fe22f9813c0 in ?? ()
#25 0x00007ffe1efeb328 in ?? ()
#26 0x0000000000000008 in ?? ()
#27 0x0000000000000007 in ?? ()
#28 0x0000000000000048 in ?? ()
#29 0x00007fe22f964070 in ?? ()
#30 0x00007fe22f9a0230 in ?? ()
#31 0x0000000000000000 in ?? ()

Comment 1

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

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20151125161010" and the hash "bc9a43cbbdfad05a533c43dcecb59527e3b22b01".
The "bad" changeset has the timestamp "20151125161413" and the hash "91c196b60306403eeb870ac65c9c81fe793e07e9".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bc9a43cbbdfad05a533c43dcecb59527e3b22b01&tochange=91c196b60306403eeb870ac65c9c81fe793e07e9

which points to bug 1226027, but it does not seem to be the regressor. I'll use the tips in:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#How_to_debug_oomTest%28%29_failures

and see what I can find.

Comment 2

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

Running with OOM_VERBOSE=1,

ReportOutOfMemory called
  allocation 517
ReportOutOfMemory called
  allocation 518
ReportOutOfMemory called
ReportOutOfMemory called
  allocation 519
  allocation 520
  finished after 519 allocations
thread 2
  allocation 1
  finished after 0 allocations
thread 3
  allocation 1
  finished after 0 allocations
thread 4
  allocation 1
  finished after 0 allocations
thread 5
  allocation 1
  finished after 0 allocations
thread 6
  allocation 1
  finished after 0 allocations
thread 7
  allocation 1
  finished after 0 allocations
Segmentation fault (core dumped)


So I'm not sure which allocation to break at, as they all seem to be "finished", so setting needinfo? from Jon, who might know the best way to proceed.

Comment 3

[Jon Coppeard (:jonco)]

It looks like the oomTest() part completes OK.  The crash happens executing the last line of the testcase.

The crash happens in js::jit::Invalidate():

3065	        // Keep the ion script alive during the invalidation and flag this
3066	        // ionScript as being invalidated.  This increment is removed by the
3067	        // loop after the calls to InvalidateActivation.
3068	        co->ion()->incrementInvalidationCount();
3069	        numInvalidations++;

The problem is that co->ion() == ION_DISABLED_SCRIPT here.

I couldn't see any obvious culprits with blame.  Jan, any idea?

Comment 4

[Jan de Mooij [:jandem]]

I can't reproduce this with the revision, configure flags, shell flags in comment 0. Do you know if this is Linux-only?

Comment 5

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

Yes, this seems to be Linux-only. I reproduced this issue on m-c rev 9ddf0da90fb3 on Linux but not on Mac.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> Yes, this seems to be Linux-only. I reproduced this issue on m-c rev
> 9ddf0da90fb3 on Linux but not on Mac.

Thanks. I'll forward this to nbp, maybe he can reproduce this.

Comment 7

[Nicolas B. Pierron [:nbp]]

I am able to reproduce this issue, I will investigate more on Monday.

Comment 8

[Nicolas B. Pierron [:nbp]]

The problem is located inside CodeGenerator::link and comes from the fact that discardIonCode is not immediately created after FinishCompilation.  Thus when the OOMTest fucntion cause a failure under encodeSafepoints function, we have not yet invalidated the RecompileInfo structure.

I suggest that we move the FinishCompilation function call to a method call of AutoDiscardIonCode, such that this kind of error cannot happen in the future.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: CodeGenerator: Properly indent IonScript::New.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: CodeGenerator: Prepare the invalidation of the recompileInfo as soon as the contraints are recorded.

This patch replace AutoDiscardIonCode by 2 uses of MakeScopeExit.  I first
tried to merge both into AutoDiscardIonCode, and failed to come-up with a
proper name for AutoDiscardIonCode, as it does not discard any IonCode
(directly).

As there is only one use AutoDiscardIonCode, using MakeScopeExit instead
should be easier to understand, and relies on RAII the same way to execute
the lambda given as argument unless released.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8704622 [details] [diff] [review]
CodeGenerator: Prepare the invalidation of the recompileInfo as soon as the contraints are recorded.

Review of attachment 8704622 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/jit/CodeGenerator.cpp
@@ +8084,5 @@
>      // will trickle to jit::Compile() and return Method_Skipped.
>      uint32_t warmUpCount = script->getWarmUpCount();
> +
> +    // Record constraints. If an error occured, returns false and potentially
> +    // prevent future compilations. Otherwise, if the an invalidation occurs,

Nit: s/the an/an/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dce0db8de74
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcf131a146d

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/182c36f37bf2

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dce0db8de74
https://hg.mozilla.org/mozilla-central/rev/8dcf131a146d
https://hg.mozilla.org/mozilla-central/rev/182c36f37bf2