944080 - Assertion failure: false, at ../vm/SelfHosting.cpp:136 with "non-canonical BestAvailableLocale locale"

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

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


function intLength (a, l) {
  var res = 0;
  for (var i = 0; i < l; i++)
      res += a.length;
}
var denseArray = [0,1,2,3,4,5,6,7,8,9];
var typedArray = new Uint8Array(10);
intLength(denseArray, 10)
intLength(typedArray, 10)
Object.prototype.length = function () {};
var input = [];
collator = new Intl.Collator("en-US");
input.sort(collator.compare)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: delta.diff

This runs without asserting without --ion-eager, so I doubt it's an Intl issue.

Something's going seriously wrong inside of CanonicalizeLanguageTag when Ion gets ahold of this, looks like.  If I apply this patch, I get output like this:

Starting program: /home/jwalden/moz/slots/js/src/dbg/js --ion-eager
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7ffff1aa8700 (LWP 23290)]
js> function intLength (a, l) {
  var res = 0;
  for (var i = 0; i < l; i++)
      res += a.length;
}
[New Thread 0x7ffff12a7700 (LWP 23291)]
[New Thread 0x7ffff10ff700 (LWP 23292)]
[New Thread 0x7ffff0eff700 (LWP 23293)]
[New Thread 0x7ffff0e7e700 (LWP 23294)]
[New Thread 0x7ffff0dfd700 (LWP 23295)]
[New Thread 0x7ffff0d7c700 (LWP 23296)]
[New Thread 0x7ffff0cfb700 (LWP 23297)]
[New Thread 0x7ffff0c7a700 (LWP 23298)]
js> var denseArray = [0,1,2,3,4,5,6,7,8,9];
js> var typedArray = new Uint8Array(10);
js> intLength(denseArray, 10)
js> intLength(typedArray, 10)
js> Object.prototype.length = function () {};
(function () {})
js> var input = [];
js> collator = new Intl.Collator("en-US");

JSString* (0x7ffff0f56100) = jschar * (0x7ffff0f56110) = "tag before: en-US"

JSString* (0x7ffff0f56300) = jschar * (0x7ffff0f56310) = "subtags before: en,us"

JSString* (0x7ffff0f56380) = jschar * (0x7ffff0f56390) = "i after: 0"

JSString* (0x7ffff0f563c0) = jschar * (0x7ffff0f563d0) = "subtags after: en,us"

JSString* (0x7ffff112da80) = jschar * (0x7ffff112da90) = "normal: "

JSString* (0x7ffff0f56440) = jschar * (0x7ffff0f56450) = "tag after: en-us"

JSString* (0x7ffff0f56480) = jschar * (0x7ffff0f56490) = "list: en-us"

Collator
js> input.sort(collator.compare)
JSString* (0x7ffff0f56600) = jschar * (0x7ffff0f56610) = "subtags before: en,us"

JSString* (0x7ffff0f56640) = jschar * (0x7ffff0f56650) = "i after: 2"

JSString* (0x7ffff0f56680) = jschar * (0x7ffff0f56690) = "subtags after: en,US"

JSString* (0x7ffff0f566c0) = jschar * (0x7ffff0f566d0) = "normal: en-US"

Self-hosted JavaScript assertion info: "non-canonical BestAvailableLocale locale"
Assertion failure: false, at ../vm/SelfHosting.cpp:137

So the |while (i < subtags.length)| loop isn't even running at all, because something seemingly thinks |subtags.length| is 0, or evaluates to that after implicit coercions.  But |subtags| is the result of a String.prototype.split call, so it's an array, so the change to Object.prototype.length should have no effect on it.  Hmm.

Comment 2

[Jan de Mooij [:jandem]]

Here's a test that does not use Intl and fails with --ion-eager.

Brian, it looks like the "updateObserved" path in jit::PropertyReadNeedsTypeBarrier is incorrectly adding the function assigned to Object.prototype.length to the observed typeset. Then, IonBuilder::getPropTryConstant sees this singleton and calls testSingletonPropertyTypes.

In testSingletonProperty, we should notice array.length shadows Object.prototype.length but we don't, I think that's the bug. But PropertyReadNeedsTypeBarrier is also incorrectly updating the observed types. Brian, can you take a look?

function intLength (a, l) {
    var res = 0;
    for (var i = 0; i < l; i++)
	res += a.length;
}
intLength([0,1,2,3,4,5,6,7,8,9], 10)
intLength(new Uint8Array(10), 10)
function test() {
    var a = "abc".split("");
    var res = 0;
    for (var i=0; i<20; i++)
	res += a.length;
    return res;
}
Object.prototype.length = function(){};
assertEq(test(), 60);

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

This fixes the testSingletonProperty issue, though PropertyReadNeedsTypeBarrier still populates the observed types incorrectly.  Fixing the latter issue would best be done by moving the type barrier logic to IonBuilder so that ClassHasEffectlessLookup/ClassHasResolveHook can be called.

Comment 4

[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/636620b3af0a
user:        Brian Hackett
date:        Tue Oct 29 16:10:59 2013 -0600
summary:     Bug 930048 - Remove need to read objects directly when optimizing singleton accesses, r=jandem.

This iteration took 1.517 seconds to run.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5f8c6d9f3b

Comment 6

[:Benjamin Peterson]

Comment on attachment 8340668 [details] [diff] [review]
patch

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

::: js/src/jit/IonBuilder.cpp
@@ +5986,5 @@
> +    // While arrays do not have resolve hooks, the types of their |length|
> +    // properties are not reflected in type information, so pretend there is a
> +    // resolve hook for this property.
> +    if (clasp == &ArrayObject::class_)
> +        return name = comp->runtime()->names().length;

This looks wrong to me. (assignment instead of comparison)

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b5f8c6d9f3b

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to :Benjamin Peterson from comment #6)
> > +        return name = comp->runtime()->names().length;
> 
> This looks wrong to me. (assignment instead of comparison)

Looks wrong to me too. (Note that "name" is local to this function, which goes away as soon as we return, so the assignment has no effect)

This also spams a build warning:
{
js/src/jit/IonBuilder.cpp:6005:48: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
}

needinfo=brian to be sure he notices & addresses it. (I'm assuming it wants to be ==, as Benjamin said)

Comment 9

[Brian Hackett [Laid off!]]

Oops, yes, this should use ==.

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

Comment 10

[Daniel Holbert [:dholbert]]

Great, thanks.

We should make sure that followup gets into Aurora (which branches tomorrow).  (I don't imagine it'll be a problem, but it's conceivable that it could miss the merge, due to inbound-->central-->holly merge timing vs. the aurora branch timing.)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6b7cc20cf99

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> We should make sure that followup gets into Aurora (which branches
> tomorrow).

er, s/tomorrow/today/.

And the followup did not quite make the merge -- Aurora has the single-"=", as shown by this 'annotate' view of current aurora tip: https://hg.mozilla.org/releases/mozilla-aurora/annotate/e0c328d99742/js/src/jit/IonBuilder.cpp#l6006

bhackett, can you make sure the followup patch gets to Aurora? (technically it needs approval, but you probably can get a release driver to casually grant it given the trivial nature)

Comment 13

[Brian Hackett [Laid off!]]

Attachment: fix typo

[Approval Request Comment]
This is a typo fix that didn't make the aurora cutoff.

Comment 14

[Ioana (away)]

https://hg.mozilla.org/mozilla-central/diff/6b5f8c6d9f3b/js/src/jit-test/tests/ion/bug944080.js