Closed Bug 1001368 Opened 6 years ago Closed 6 years ago

[jsdbg2] Assertion failure: is<T>(), at jsobj.h:1128 or Crash [@ settle]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- affected

People

(Reporter: decoder, Assigned: shu)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(3 files, 1 obsolete file)

The following testcase asserts on mozilla-central revision 5ecd532a167e (run with --fuzzing-safe --ion-eager):


var g = newGlobal();
g.debuggeeGlobal = this;
g.eval("(" + function () { dbg = new Debugger(debuggeeGlobal); } + ")();");
for (var i4 = 0; i4 < 16; i4++) {
  try {
    var pattern = eval("/" + xx + "/");
  } catch (e) {
    $ERROR('#' + uu + ' ');
  }
}
Crash Signature: [@ settle]
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Setting NI on myself as reminder.
Flags: needinfo?(shu)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140424014842" and the hash "25dd3d17a19a".
The "bad" changeset has the timestamp "20140424020245" and the hash "079f4f0ed6a6".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25dd3d17a19a&tochange=079f4f0ed6a6
This is an existing bug in the Baseline implementation of Debugger hooks.

When handling exceptions in baseline, we call UnwindScope. DebugEpilogue also calls UnwindScope. This can cause inconsistencies, because we're unwinding scope without changing the pc. In the interpreter, after unwinding the scope to a certain pc, we jump to that pc. In the baseline, if we're handling an exception that has no catch/finally, we call DebugEpilogue and terminate immediately, while the BaselineFrame's pc is still settled on the instruction that caused the exception. This breaks a bunch of Debugger assumptions.
Flags: needinfo?(shu)
Attached patch TestsSplinter Review
Tests to reproduce this without any debug mode OSR stuff.
Attachment #8414204 - Flags: review?(jimb)
Assignee: nobody → shu
Status: NEW → ASSIGNED
See comment 4 for explanation. I'm not entirely happy with this approach, but I
can't think of a better way.
Attachment #8414205 - Flags: review?(jdemooij)
v2. Turns out the override pc can very well coexist with a DebugModeOSRInfo on
the same frame. Use the previously unused padding to store the override pc offset.
Attachment #8414205 - Attachment is obsolete: true
Attachment #8414205 - Flags: review?(jdemooij)
Attachment #8414209 - Flags: review?(jdemooij)
Comment on attachment 8414209 [details] [diff] [review]
Fix UnwindScope logic in BaselineHandleException to account for Debugger.

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

::: js/src/jit/BaselineFrame.h
@@ -81,4 @@
>      uint32_t flags_;
> -#if JS_BITS_PER_WORD == 32
> -    uint32_t padding_;              // Pad to 8-byte alignment.
> -#endif

Ah good this change doesn't affect the frame size.
Attachment #8414209 - Flags: review?(jdemooij) → review+
Comment on attachment 8414204 [details] [diff] [review]
Tests

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

::: js/src/jit-test/tests/debug/Frame-onPop-error-scope-unwind-01.js
@@ +6,5 @@
> +dbg.onEnterFrame = function (f) {
> +  if (f.callee && f.callee.name == "f") {
> +    f.onPop = function() {
> +      // The scope at the point of onPop is the outermost.
> +      correct = f.environment.getVariable("e") === undefined;

Would it make sense to also check for a binding we *do* expect to be there? Because "doesn't contain a binding for 'e'" could probably describe most environments floating around in the system.

@@ +23,5 @@
> +
> +try {
> +  g.eval("f();");
> +} catch (e) {
> +  // Don't crash.

I think you mean, "The above is expected to throw."  Saying "don't crash" is ambiguous, since this was a crasher bug.
Attachment #8414204 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/ef50feec881a
https://hg.mozilla.org/mozilla-central/rev/a0619a71579d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.