Closed Bug 1063488 Opened 5 years ago Closed 5 years ago

Picture do not appears.

Categories

(Core :: JavaScript Engine: JIT, defect)

34 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 + fixed
firefox35 + fixed
firefox-esr31 --- unaffected
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: alice0775, Assigned: bhackett)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

[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.
Oops,

Expected Results:
Screen repainted to a mosaic picture(Mona Lisa)
Setting javascript.options.baselinejit = false or javascript.options.ion = false helps
Component: JavaScript Engine → JavaScript Engine: JIT
Attached patch patchSplinter Review
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.
Assignee: nobody → bhackett1024
Attachment #8485155 - Flags: review?(jdemooij)
(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?
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 on attachment 8485155 [details] [diff] [review]
patch

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

Thanks for the testcase/explanation. Good catch.
Attachment #8485155 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/e4677757051d
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1067703
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
Attachment #8485155 - Flags: approval-mozilla-aurora?
Comment on attachment 8485155 [details] [diff] [review]
patch

Aurora+
Attachment #8485155 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached image 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
You need to log in before you can comment on or make changes to this bug.