885660 - JSIL raytracer demo fails to load unless debugger is active

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Katelyn Gadd (:kael)]

Attachment: Test case

A recent change to the JS engine has caused the JSIL raytracer demo to fail to load unless the built-in debugger is active.

It fails with this error message:

Unhandled exception at http://127.0.0.1:81/Libraries/JSIL.Core.js line 6719:
TypeError: assembly is undefined

If the built in debugger is active the page loads normally and no exceptions (handled or otherwise) are thrown.

The test case still works in IE and older builds of Firefox.

Mozregression window:
Last good nightly: 2013-05-08
First bad nightly: 2013-05-09

Comment 1

[Loic]

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b980d32c366f&tochange=ea059733677c

Suspected bug:
Boris Zbarsky — Bug 869040. Fix ion IC for non-overridebuiltins named gets on ListBase proxies to not cache lack of a property when it's just missing on the prototype. r=djvj

Workaround:
javascript.options.ion.content = false fixes the issue.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ccing the people who actually know jit stuff...

Comment 3

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/dea5219d6b43
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130508 Firefox/23.0 ID:20130508210518
Bad:
http://hg.mozilla.org/mozilla-central/rev/ea059733677c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130508 Firefox/23.0 ID:20130508211818
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dea5219d6b43&tochange=ea059733677c

Regression window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1c05e7a9e51c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130508 Firefox/23.0 ID:20130508095549
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/469e647ccbf5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130508 Firefox/23.0 ID:20130508104708
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1c05e7a9e51c&tochange=469e647ccbf5

Comment 4

[Jan de Mooij [:jandem]]

Bug 868042 regressing this is unlikely, it probably exposed an existing bug. I'll investigate.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Shell testcase

Small shell testcase. Throws an error with default flags. Does not throw with --no-ion. We are bailing out in JSIL.GetTypeInternal and somehow context becomes |undefined|.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

What's happening is that we have a snapshot like this:

formal x: undefined (constant)
local y : LArgument (x's slot)

We bailout and overwrite the argument on the stack with UndefinedValue(). When we get to the local, we read the |undefined| Value from the stack instead of the original Value that was stored there. The argument is never really undefined, but due to UCE / phi elimination we think the formal is unused when we bailout and store undefined in the snapshot.

The patch fixes this by storing the formals in a Vector and only writing them to the stack at the end of the bailout.

Comment 7

[Kannan Vijayan [:djvj]]

Comment on attachment 766702 [details] [diff] [review]
Patch

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

Nice find.

Comment 8

[Jan de Mooij [:jandem]]

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

Comment 9

[Loic]

Which bug regressed that?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Loic from comment #9)
> Which bug regressed that?

I think the Baseline Compiler landing, though for this particular testcase it was only a problem after some other changes landed.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 766702 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Most likely the new Baseline compiler (introduced in Firefox 23)
User impact if declined: Correctness bugs, probably NULL-deref crashes
Testing completed (on m-c, etc.): On m-i
Risk to taking this patch (and alternatives if risky): Very low
String or IDL/UUID changes made by this patch: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3bab1b490e9f
https://hg.mozilla.org/releases/mozilla-beta/rev/22329b2bff99

Comment 14

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20130917123208

Verified as fixed on Firefox 25 beta 1 - the raytracer demo loads fine without an active debugger.