800179 - IonMonkey: "Assertion failure: !usedRval_,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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.

Comment 1

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

Attachment: stack

Comment 2

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

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

Comment 3

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

Jan has taken a look at this at my computer and may have a solution, tentatively assigning.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5beb8aac8b47

Comment 7

[(no longer active)]

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 <[e-mail]> - 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 <[e-mail]> - Bug 800179 - Initialize CallArgsList::active_ in the constructor. r=luke
    : http://bugzilla.mozilla.org/show_bug.cgi?id=800179

Comment 8

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

I guess this needs a backout then..

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't see any regression on AWFY.

Comment 10

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

(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.

Comment 11

[Jan de Mooij [:jandem]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5beb8aac8b47

Comment 13

[(no longer active)]

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 14

[Jan de Mooij [:jandem]]

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

Comment 15

[Jan de Mooij [:jandem]]

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.

Comment 16

[bhavana bajaj [:bajaj]]

(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 !

Comment 17

[Jan de Mooij [:jandem]]

(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).

Comment 18

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8914200121e9

Comment 19

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

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.