925308 - Assertion failure: *(int*)size_ >= 0, at vm/SPSProfiler.cpp:196

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


var lfcode = new Array();
lfcode.push("3");
lfcode.push("enableSPSProfilingAssertions(false);foo();");
while (true) {
  var file = lfcode.shift(); if (file == undefined) { break; }
  loadFile(file)
}
function loadFile(lfVarx) {
    if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) {
        switch (lfRunTypeId) {
            default: function newFunc(x) { new Function(x)(); }; newFunc(lfVarx); break;
        }
    } else if (!isNaN(lfVarx)) {
        lfRunTypeId = parseInt(lfVarx);
    }
}

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

This is causing quite a few signatures in the fuzzer, including a critical heap crash. Marking as fuzzblocker.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/88a218a4b5bf
user:        Jan de Mooij
date:        Tue Dec 25 16:12:59 2012 +0100
summary:     Bug 764310 part 2 - Implement JSOP_DEFFUN in IonMonkey. r=bhackett

This iteration took 144.623 seconds to run.

Comment 4

[Christian Holler (:decoder)]

Seeing this on beta as well.

Comment 5

[Andrew McCreight [:mccr8]]

Marking sec-moderate because it looks like it requires SPS, but it still sounds pretty bad...

Comment 6

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 7

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9f8233fcce1d).

Comment 8

[Christian Holler (:decoder)]

I filed this before and it stopped reproducing. Please investigate this on the original revision and fix it if required ,thanks.

Comment 9

[Kannan Vijayan [:djvj]]

Gak!  This is a hairy issue.  Here's what happens:

1. We compile Ion code before SPS is enabled, enter the compiled Ion code, and do a VM-call to InvokeFunction a JS function.
2. Inside the InvokeFunction, code executes which turns profiling on, invalidating the original compiled IonCode.
3. Before returning, though.. an exception is raised.  This exception is handled by the VM-call's trampoline, which occurs BEFORE the patched OSI-point for the invalidated IonCode is hit.  It sees the current profiling state as "on" and tries to pop an SPS frame (which doesn't exist, since it was pushed by non-SPS enabled Ion code that has since been invalidated).

Jan, I need to talk with you about potential solutions to this.  I have the beginnings of one sketched out, but I'd like to run it by you.

Comment 10

[Kannan Vijayan [:djvj]]

After discussing this with :jandem, he suggested maybe changing probes::ExitScript to take a ScriptFrameIter instead of an AbstractFramePtr, and thread the APIs for ScriptFrameIter such that the SPS information was available from it.

Thinking more about it, I realized that the only thing probes::ExitScript uses from the frame is the flag on whether it needs to pop an SPS frame or not.  It seemed a lot easier and lightweight to have the callsite pass in just that flag, so that's what I did.

Comment 11

[Kannan Vijayan [:djvj]]

Attachment: sps-bug-925308.patch

See comment 10 for description of fix.

Confirmed that it worked on the bug on the revision where bug was first discovered.  Bug no longer reproduces on tip, so I'm not sure if I should include the test case or not.

Comment 12

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

> Confirmed that it worked on the bug on the revision where bug was first
> discovered.  Bug no longer reproduces on tip, so I'm not sure if I should
> include the test case or not.

Yes, we should, in case it reappears again.

Comment 13

[Kannan Vijayan [:djvj]]

Noted.  Added test case to jit-tests.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 828891 [details] [diff] [review]
sps-bug-925308.patch

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

Nice.

::: js/src/jit/IonFrames.cpp
@@ +552,5 @@
>              for (;;) {
>                  HandleExceptionIon(cx, frames, rfe, &overrecursed);
>  
> +                IonScript *ionScript = nullptr;
> +                bool invalidated = iter.checkInvalidation(&ionScript);

We can move these two lines out of this loop, since |invalidated| will have the same value for all inlined frames.

Then we can change these lines after the loop:

    IonScript *ionScript = nullptr;
    if (iter.checkInvalidation(&ionScript))
        ionScript->decref(cx->runtime()->defaultFreeOp());

To:

    if (invalidated)
        ionScript->decref(cx->runtime()->defaultFreeOp());

r=me with that.

Comment 15

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e75f3902b7f6

Comment 16

[Ed Morley [:emorley]]

Backed out for assertions:
https://tbpl.mozilla.org/php/getParsedLog.php?id=30421818&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=30421904&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1f96641beb14

Comment 17

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

(In reply to Ed Morley [:edmorley UTC+1] from comment #16)
> Backed out for assertions:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30421818&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30421904&tree=Mozilla-
> Inbound
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1f96641beb14

Looks like it also causes browser-chrome crashes: https://tbpl.mozilla.org/php/getParsedLog.php?id=30425856&tree=Mozilla-Inbound
and xpcshell crashes: https://tbpl.mozilla.org/php/getParsedLog.php?id=30425558&tree=Mozilla-Inbound

Comment 18

[Kannan Vijayan [:djvj]]

Re-pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/34001410d373

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=9b0ae218a64d

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34001410d373

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please land the test.

Comment 21

[Kannan Vijayan [:djvj]]

Sorry, forgot to 'hg add' the file.

https://hg.mozilla.org/integration/mozilla-inbound/rev/85e15ff221ca

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Test: https://hg.mozilla.org/mozilla-central/rev/85e15ff221ca

Comment 24

[Alex Keybl [:akeybl]]

For now this sec-moderate is a wontfix, unless others would like to nominate for ESR24 approval. Unclear that this is desirable on the ESR branch.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Should this be uplifted to Fx27 still? What about b2g18* or b2g26?

Comment 26

[Al Billings [:abillings - ex-MoCo]]

27 should get it. ESR has a different bar.

Comment 27

[Kannan Vijayan [:djvj]]

Comment on attachment 828891 [details] [diff] [review]
sps-bug-925308.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Hard to identify.  Probably there since profiler instrumentation.

User impact if declined: 
Potential for bad memory writes at fixed address, leading to crash.

Testing completed (on m-c, etc.): 
3-weeks green on m-c.

Risk to taking this patch (and alternatives if risky):
Low.
 
String or IDL/UUID changes made by this patch:
N/A

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/37df29dd2f8f

I'm guessing b2g18 is wontfix for this, but what about b2g26?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Kannan, is this worth trying to uplift to the B2G branches?

Comment 30

[Kannan Vijayan [:djvj]]

I'd guess no.  This is a rare corner case that only hits when profiling is switched on or off in very particular ways, deep within a call stack.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Sounds that way, thanks :)