IonMonkey: Crash [@ js::HeapPtr<js::ion::IonCode, unsigned long>::operator js::ion::IonCode*] or [@ checkInvalidation] or [@ js::ion::FrameRecovery::FromIterator]

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 1 bug, {crash, testcase})

Other Branch
x86_64
Linux
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 610026 [details]
stacks

function List(l) {
  this.l = l;
}
function f(p) {
  return g(p.l);
};
function g(p) {
  return !(p instanceof List) ? null : f(p.l);
};
list =
  new List(new List(
    new List(new List(
      new List(new List(
        new List(new List(null))))))))
for (let i = 0; i < 99999; i++) {
  g(list);
}

crashes js debug shell on IonMonkey changeset 940078281bbf with --ion and -n at js::HeapPtr<js::ion::IonCode, unsigned long>::operator js::ion::IonCode* and crashes js opt shell at checkInvalidation with js::ion::FrameRecovery::FromIterator on the stack.

Nicolas had the original super-large 5,000+ line testcase and I helped him reduce it.
(Reporter)

Comment 1

5 years ago
Created attachment 610027 [details]
Valgrind stack
(Reporter)

Updated

5 years ago
Blocks: 732847
(Reporter)

Comment 2

5 years ago
We decided to look at it for a few more minutes (again!):

function List(l) {
    this.l = l;
}
function f(p) {
    g(p);
};
function g(p) {
    if (p instanceof List) f(p.l);
};
list = new List(new List(new List(new List(null))))
for (let i = 0; i < 2560; i++)
    g(list);
(Assignee)

Comment 3

5 years ago
What happens:
* We fail to compile g due to the lack of instanceof.
* We compile f.
* We hit recompilecheck bailout in f while having entered f twice before.
* We bailout, js_DumpBacktrace show 3 IonFrames before.
* we finish to bailout and ion frame have disappeared from js_DumpBacktrace output.
* we return to the interpreter where we compile f again.
* we try to inline g, and fail due to instanceof, and flag f as disabled.
* we return from the interpreter, and call the monitor function.
* the monitor function use GetPcScript and recover the bailout ionscript with checkInvalidation which segv.

This bug contains 3 errors:

1/ CheckInvalidation does not support when script->ion is set ION_DISABLED_SCRIPT.

2/ js_DumpBacktrace, which depends on checkInvalidation, is unable to display Ion frames (same as 1 ?)

3/ We should not allow to inline a script which have failed to compiled previouly



The first error fix is needed to land Bug 732847.
(Assignee)

Comment 4

5 years ago
Created attachment 610476 [details] [diff] [review]
[part 1] checkInvalidation handle script which are no-longer compilable

This patch fix the issue which prevent the landing of bug 732847 fix, which is blocking decoder fuzzing.

It makes sure we don't try to access DISABLE scripts properties when checking for invalidated code.
Attachment #610476 - Flags: review?(dvander)
Comment on attachment 610476 [details] [diff] [review]
[part 1] checkInvalidation handle script which are no-longer compilable

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

Looks good to me.
Attachment #610476 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

5 years ago
part 1: https://hg.mozilla.org/projects/ionmonkey/rev/9d13b69838c2
part 2 & 3 (?) still have to be fixed.
(Assignee)

Comment 7

5 years ago
Created attachment 610702 [details] [diff] [review]
[part 2] Do not inline uncompilable functions

Check if inline candidates are disabled or not.
Attachment #610702 - Flags: review?(sstangl)
(Assignee)

Comment 8

5 years ago
Comment on attachment 610702 [details] [diff] [review]
[part 2] Do not inline uncompilable functions

This patch has moved to its own bug (Bug 740622)
Attachment #610702 - Flags: review?(sstangl)
Version: Trunk → Other Branch
(Assignee)

Comment 9

5 years ago
Created attachment 611515 [details] [diff] [review]
[part 2] Fix StackIter iteration over empty Ion activations

DumpBacktrace (from GDB) and dumpStack (from the shell) was not counting the right amount of frame just after the bailout caused by recompilecheck.  The reason was that StackIter did not increment the IonActivation iterator when it encounter an empty activation (Entry | Exit) which is caused by the bailout of the first Ion frame of the activation.

The test case is a modified version of the bug reported here, such as it returns the length of the stack to check for the current bug.
Attachment #610702 - Attachment is obsolete: true
Attachment #611515 - Flags: review?(dvander)
Attachment #611515 - Flags: review?(dvander) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/291ff6ed10b5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.