Closed Bug 800179 Opened 8 years ago Closed 8 years ago

IonMonkey: "Assertion failure: !usedRval_,"

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox15 --- unaffected
firefox16 --- unaffected
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: gkw, Assigned: jandem)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [ion:p1:fx19][adv-main18-])

Attachments

(2 files, 1 obsolete file)

try {
    x = []
    y = function() {}
    t = Uint8ClampedArray
    Object.defineProperty(x, 1, {
        get: (function() {
            for (v of t) {}
        })
    })
    Object.defineProperty(x, 8, {
        configurable: t
    }).reverse()
} catch (e) {}
Object.defineProperty([], 1, {
    configurable: true,
    get: (function() {
        for (j = 0; j < 50; ++j) {
            y()
        }
    })
}).pop()
x.map(y)

asserts js debug shell from m-c nightly js shell http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-10-10-mozilla-central-debug/jsshell-win32.zip - m-c changeset ec10630b1a54 with --no-jm at Assertion failure: !usedRval_,

The assertion goes away when adding --no-ion, so assuming IonMonkey related.

s-s because I don't know how bad this is, moreover it seems platform-dependent.
Attached file stack
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   109579:d407f51ca61e
user:        Jan de Mooij
date:        Sun Oct 07 14:27:11 2012 -0700
summary:     Bug 798823 - Don't use an empty IonActivation in FastInvokeGuard. r=dvander
Blocks: 798823
Summary: "Assertion failure: !usedRval_," → IonMonkey: "Assertion failure: !usedRval_,"
Whiteboard: [ion:p1] → [ion:p1:fx19]
Jan has taken a look at this at my computer and may have a solution, tentatively assigning.
Assignee: general → jdemooij
Flags: needinfo?(jdemooij)
Attached patch Patch (obsolete) — Splinter Review
InvokeArgsGuard inherits from CallArgsList, and CallArgsList::active_ is not initialized. Before I added FastInvokeGuard, Invoke(InvokeArgsGuard) would always call setInactive(). However, if FastInvokeGuard does not call Invoke but directly calls into Ion, "active_" stays uninitialized and this causes assertion failures in StackIter. This patch adds a constructor to initialize these members.
Attachment #675831 - Flags: review?(luke)
Flags: needinfo?(jdemooij)
Attachment #675831 - Flags: review?(luke)
Attached patch PatchSplinter Review
Like the previous patch (see comment 4) but also calls setActive/setInactive before calling into Ion. I don't think we call active() anywhere other than checking for native calls atm though.
Attachment #675831 - Attachment is obsolete: true
Attachment #676206 - Flags: review?(luke)
Attachment #676206 - Flags: review?(luke) → review+
Regression  Kraken Benchmark increase 3.72% on XP Mozilla-Inbound-Non-PGO
---------------------------------------------------------------------------
    Previous: avg 3830.580 stddev 9.166 of 30 runs up to revision b3b933d6790d
    New     : avg 3972.940 stddev 2.955 of 5 runs since revision 5beb8aac8b47
    Change  : +142.360 (3.72% / z=15.531)
    Graph   : http://mzl.la/UbccYA

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b3b933d6790d&tochange=5beb8aac8b47

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/2d8d8ceb7a6a
    : James Willcox <jwillcox@mozilla.com> - Bug 806343 - Avoid empty define for MOZ_PKG_SPECIAL r=blassey
    : http://bugzilla.mozilla.org/show_bug.cgi?id=806343

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/5beb8aac8b47
    : Jan de Mooij <jdemooij@mozilla.com> - Bug 800179 - Initialize CallArgsList::active_ in the constructor. r=luke
    : http://bugzilla.mozilla.org/show_bug.cgi?id=800179
I guess this needs a backout then..
I don't see any regression on AWFY.
(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> Regression  Kraken Benchmark increase 3.72% on XP Mozilla-Inbound-Non-PGO
> ---------------------------------------------------------------------------
>     Previous: avg 3830.580 stddev 9.166 of 30 runs up to revision
> b3b933d6790d
>     New     : avg 3972.940 stddev 2.955 of 5 runs since revision 5beb8aac8b47
>     Change  : +142.360 (3.72% / z=15.531)
>     Graph   : http://mzl.la/UbccYA

Ehsan posts a regression in Kraken on WinXP.

IIRC AWFY does not run on WinXP, so it may be a Windows-only issue.

Moreover this bug manifested for me only on Windows.
Kraken doesn't use FastInvoke. That means it would have to be the initialization of CallArgsList::active_ and CallArgsList::prev_, that seems very unlikely and these benchmark numbers have always been noisy.

Please don't back this out before manually verifying this, I can test on Windows 7 later today.
https://hg.mozilla.org/mozilla-central/rev/5beb8aac8b47
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
FWIW, for some reason the Kraken benchmark on XP is more sensitive to performance changes.  I investigated a patch a couple of weeks ago which was regressing Kraken on XP, and I felt similarly to you at first, but it turned out that the patch was changing the compiler inlining decisions and the regression was real.
Comment on attachment 676206 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 797131
User impact if declined: Control flow depends on an uninitialized value, caused an assertion failure in this bug but may also crash -- hard to say for sure
Testing completed (on m-c, etc.): On m-c for a week
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None
Attachment #676206 - Flags: approval-mozilla-aurora?
I have no idea how to investigate the reported WinXP Kraken regression (see comment 7 and comment 11). The few lines of code this patch changes are either not used at all on Kraken, or are not hot. I'm pretty sure this regression is bogus and since this bug is security sensitive and initializing these values to sane defaults is something we really want to do, I want ahead and requested approval for aurora.
(In reply to Jan de Mooij [:jandem] from comment #15)
> I have no idea how to investigate the reported WinXP Kraken regression (see
> comment 7 and comment 11). The few lines of code this patch changes are
> either not used at all on Kraken, or are not hot. I'm pretty sure this
> regression is bogus and since this bug is security sensitive and
> initializing these values to sane defaults is something we really want to
> do, I want ahead and requested approval for aurora.

Would be helpful to know if this patch was verified with some manual tests as per comment 11 to be safe ? Thanks !
Flags: needinfo?(jdemooij)
(In reply to bhavana bajaj [:bajaj] from comment #16)
> 
> Would be helpful to know if this patch was verified with some manual tests
> as per comment 11 to be safe ? Thanks !

I compared Windows 32-bit non-PGO builds (like the one for which the regression was reported) with and without the patch. I ran Kraken 1.1 several times on Windows 7 and I got similar numbers (the numbers varied between 1730-1740 ms with both builds).
Flags: needinfo?(jdemooij)
Attachment #676206 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
Whiteboard: [ion:p1:fx19] → [ion:p1:fx19][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.