925470 - JSIL game "Soulcaster" fails to parse XML level description

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load the JSIL game "Soulcaster": http://www.newgrounds.com/portal/view/606067
2. Select "New Game"
3. Select "Normal"

RESULT:
The game screen goes black and the game level does not load. The following JS error is logged:

JavaScript error: http://uploads.ungrounded.net/alternate/623000/623433_alternate_372.zip/17/Libraries/JSIL.XML.js, line 112: Error: Failed to parse XML document

This bug is a regression in Nightly 27 build 2013-10-01. I bisected the mozilla-inbound builds archived on TBPL and identified bug 921120 as the regression:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4184c8516ee5&tochange=2963a336e7ec

This XML error is thrown from JSIL's JS code here:

JSIL.XML.ReaderFromString = function (xml) {
    var parser = new DOMParser;
    var root = parser.parseFromString(xml, 'application/xml');
    if (root === null || root.documentElement.localName == 'parsererror') {
        throw new Error('Failed to parse XML document')
    }
    ...

Comment 1

[Chris Peterson [:cpeterson]]

"Bytown Lumberjack", another JSIL-compiled game, also fails to load a new game starting in Nightly 27 build 2013-10-01 with the following error message:

  Unhandled exception at line 0:
  uncaught exception: System.ArgumentOutOfRangeException: index

http://hildr.luminance.org/Lumberjack/Lumberjack.html

Comment 2

[Kannan Vijayan [:djvj]]

Looking at it.

Comment 3

[Kannan Vijayan [:djvj]]

Is there a known way to get a local install of Bytown Lumberjack so I can start mutating the JS to narrow down the issue.

I think the lumberjack game is a better candidate for tracking this issue than Soulcaster.

However the thing loads a bunch of static and dynamic files, and I tried a crude attempt at replicating the repository locally and failed.

I can't use the debugger on this since it's an Ion issue and Ion doesn't compile when the debugger turns on :)  So with debugging on the thing works perfectly (if slowly).

The other option is just for me to start instrumenting the JS engine directly and running it on the remote code, trying to identify where exceptions are thrown.

Comment 4

[Kannan Vijayan [:djvj]]

See comment 3.  Kael: Is there an easy way to get a local copy of Bytown Lumberjack to debug this issue?

Comment 5

[Katelyn Gadd (:kael)]

I sent a build to your bugmail, Kannan.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: fix-ion-argsobj-strictmode-osr.patch

Found the issue.  In strict mode, functions which have JSOP_SETARG are forced to create an ArgumentsObject to preserve the original arguments that were passed in.

When OSR-ing into functions with ArgumentsObjects present, we unconditionally took the arguments from the ArgsObj.  However, in strict mode, the actual "current" formals aren't present in the argsobj, but on the stack frame.  They should be retrieved from the stack frame instead.

The fix is a one-liner (and some commentary).

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 827453 [details] [diff] [review]
fix-ion-argsobj-strictmode-osr.patch

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

Good catch, but I definitely want a testcase for this in the tree.

I quickly looked to all other cases where we test needsArgsObj in IM and didn't found the same issue appearing there. So I think this is the only place it was forgotten.

Comment 8

[Kannan Vijayan [:djvj]]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Kannan Vijayan [:djvj]]

Comment on attachment 827453 [details] [diff] [review]
fix-ion-argsobj-strictmode-osr.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 921120
 
User impact if declined: 
Incorrect behavior of strict-mode code that assigns to formal arguments, and contains loops.  This bug break JSIL and causes JSIL-run games not to work.

Testing completed (on m-c, etc.): 
Green for 4 days on m-c.

Risk to taking this patch (and alternatives if risky):
Very low.
 
String or IDL/UUID changes made by this patch:
N/A

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/37a77047e150

Comment 12

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on latest Aurora 27.0a2 (buildID: 20131202004002).

Comment 13

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0

Verified as fixed on latest Aurora 28.0a2 (buildID: 20131212004008).