1063488 - Picture do not appears.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: regression


Steps To Reproduce:
1. Open http://paperjs.org/examples/q-bertify/

Actual Results:
Screen repainted to black

Actual Results:
Screen repainted to a mosaic picture(Mona Lisa)


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/436085b283b4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140819231327
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9605a571ca8a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140819232629
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=436085b283b4&tochange=9605a571ca8a

Regressed by: 
9605a571ca8a	Brian Hackett — Bug 934450 - Allow objects to have copy on write elements, r=billm,jandem.

Comment 1

[Alice0775 White]

Oops,

Expected Results:
Screen repainted to a mosaic picture(Mona Lisa)

Comment 2

[Alice0775 White]

Setting javascript.options.baselinejit = false or javascript.options.ion = false helps

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

It isn't valid to treat MaybeCopyElementsForWrite as non-effectful, since there could be other loads of the original COW elements pointer which are consolidated and placed before the MaybeCopyElementsForWrite.  This patch makes MaybeCopyElementsForWrite effectful so that this consolidation can't happen, and fixes the webpage for me.

Unfortunately this keeps us from being able to GVN or loop hoist MaybeCopyElementsForWrite instructions.  This doesn't affect octane, which only writes to COW elements in one place in typescript (varListCountStack, which is not accessed in a way that can be consolidated).  It does affect a couple loops in kraken (stanford-crypto-aes and stanford-crypto-ccm) though on my machine it's hard to tell whether these tests regress.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #3)
> It isn't valid to treat MaybeCopyElementsForWrite as non-effectful, since
> there could be other loads of the original COW elements pointer which are
> consolidated and placed before the MaybeCopyElementsForWrite.

What does this mean exactly? Do you have a simple testcase?

Comment 5

[Brian Hackett [Laid off!]]

Here is a testcase, which I'll check in with the patch.

function foo(a, b) {
    var x = b[0];
    for (var i = 0; i < 5; i++) {
	a[i + 1] = 0;
	x += b[i];
    }
    assertEq(x, 2);
}
function bar() {
    for (var i = 0; i < 5; i++) {
	var arr = [1,2,3,4,5,6];
	foo(arr, arr);
    }
}
bar();

foo() can consolidate the accesses to b's elements to a load which happens before those COW elements are actually copied, and later loads from those elements can be incorrect.  This is different from MConvertElementsToDoubles as even though that one overwrites the elements in place, it doesn't lead to a change in the actual numeric values that can be read later.

Comment 6

[Jan de Mooij [:jandem]]

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

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

Thanks for the testcase/explanation. Good catch.

Comment 7

[Brian Hackett [Laid off!]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Brian Hackett [Laid off!]]

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

Approval Request Comment
[Feature/regressing bug #]: bug 934450
[User impact if declined]: website breakage
[Describe test coverage new/current, TBPL]: jit-test included
[Risks and why]: none

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Aurora+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d0669f61832

Comment 12

[Elie]

Attachment: IMAG0007.jpg

This issue has been successfully verified on Flame 2.1:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2


This issue has been successfully verified on Flame 2.2:
Gaia-Rev        824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/acde07cb4e4d
Build-ID        20141125040209
Version         36.0a1
Device-Name     flame
FW-Release      4.4.2