[patch][ccla] IE10: JS engine bug causes dojo/aspect. to fail

[Eric Durocher]

This seems to be an IE10 JS engine / JIT bug.

The context where this occurs is in Dojo Mobile transitions and can be reproduced (for example) by running this test case on IE10:

dojox/mobile/tests/test_RoundRectList.html

Click the "Slide" list item, the view becomes empty instead of transitioning to another view.

After debugging this, I spotted the problem in dojo/aspect.js, more precisely lines 53-59, where the JS engine seems to get confused with the 'previous' variable and not update it correctly in some cases, and then fail to add one of the after advices in the list (which in turn causes one of the dojox/mobile slide animations to not run - I can give the details of the call chain if needed).

This is not easy to debug directly since the bug does not occur when the IE10 debugger is active. The reasons why I am convinced that this is a JS engine bug:

1. Changing the code slightly to something strictly equivalent fixes the problem. The attached patch is one variant that does not look too bad, but more stupid things like adding a "previous = previous;" statement in the loop at line 56 work too...

2. Adding traces in a string (not through console.log calls since this also fixes the bug) shows that the value of 'previous' after the loop is not the same as the last value set in the loop.

3. Does not happen in the debugger (where, I suppose, JIT is disabled)

I tried to reproduce in simpler cases, with no success. The bug can be reproduced in many Dojo Mobile tests/demos using the same code for transitions.

The proposed patch is just one possible fix, many other workarounds would work too.

The bug is absolutely not in dojo/aspect, the JIT just happens to fail here apparently, no idea why of course. Open to any ideas how to fix this differently...

[Eric Durocher]

Possible patch for IE10 / JIT problem - Eric Durocher (IBM, CCLA)

[cjolif]

Ideally this should be fixed in 1.8.2 to reasonably claim IE10 support for dojo mobile (and maybe other code that might fail against that). Please let us know what you think.

[ben hockey]

i don't have IE10 handy... but i'm curious if this bug goes away if you take away the "use strict"; in this module.

[Eric Durocher]

I tried that but the bug is still there without "use strict" ...

[Kris Zyp]

I can't reproduce this, the Slide transition seems to work fine on the IE platform preview (version 1.9.8023.6000).

[Kris Zyp]

In [30133]:

Workaround IE10's bizarre JIT bug, fixes #16425 !strict

[Kris Zyp]

I implemented a slightly different, and more compact version of the linked list traversal that seems to avoid the bug. It does indeed appear to be some bizarre JIT bug in IE10. Let me know if this looks OK, and I can stick it in 1.8.

[bill]

It fixes the failure for me (IE10, Windows 8).

Looking at the patch I wonder why we don't just keep a reference to the end of the linked listed, in addition to having a reference to the start, but I guess that's outside the scope of this ticket, especially for 1.8.

[Eric Durocher]

It works for me too, tested on Windows 7 and 8. Thanks!

[Kris Zyp]

In [30135]:

Workaround IE10's bizarre JIT bug, fixes #16425 !strict

[Colin Snover]

This issue has been reported upstream. I will update this ticket once I have more information.

[Colin Snover]

Upstream is requesting a reduced test case for this issue. Could one of you please create one?

[Kris Zyp]

I spent some time working on this, but was never able to create anything isolated. However, if you go to the test page: ​http://download.dojotoolkit.org/release-1.8.1/dojo-release-1.8.1/dojox/mobile/tests/test_RoundRectList.html

You can reproduce the problem with a relatively small snippet of code on that page:

aspect = require('dojo/aspect');
var order = [];
obj = {
	foo: function(){}
};
aspect.after(obj, "foo", function(){order.push(1)});
aspect.after(obj, "foo", function(){order.push(2)});
aspect.after(obj, "foo", function(){order.push(3)});
obj.foo();
order

And the order array will only be [1,3], instead of [1,2,3] as it should.

I also tried running this snippet at different times in the loading of the page. When the page first starts the snippet runs correctly, but at some point, I think after the widgets have been rendered, the snippet starts running incorrectly.

And again, as mentioned, if you have developer tools script debugging turned on, the issue disappears. I had also tried removing components on the page to see if anything caused it. If you only have a couple widgets it goes away, but it is not any particular widget that causes the problem, it seems more related to just the number of widgets (yet more evidence of a low-level VM problem).

[bill]

I was looking at backporting this to 1.7 but there's no test_RoundRectList.html in 1.7 to test the fix against. Is there some case in 1.7 that fails because of this bug, or some reproducible test case?

PS: it's not an issue in 1.6, because dojo/aspect didn't come until 1.7.

[cjolif]

Adrian might be able to provide sample reproducing the issue in Eric's absence.

[Adrian Vasiliu]

Test case for Dojo 1.7.

[Adrian Vasiliu]

The attached test_RoundRectList-dojo1.7.html is a slightly modified version of test_RoundRectList.html from trunk to make it runnable in Dojo 1.7. The test is to be placed in dojox/mobile/tests.

Using it, I do reproduce the bug in IE10 on Windows 8 using Eric's testing scenario (clicking the "Slide" list item, it transitions to an empty view).

Also tested that applying the patch to dojo/aspect the bug goes away.

[bill]

Hmm, that's strange, it's working for me, see ​http://bill.dojotoolkit.org/1.7/dojox/mobile/tests/test_RoundRectList-dojo1.7.html.

[Adrian Vasiliu]

For me, your URL does reproduce it. My test env.: IE 10.0.9200.16466 under a Win8 Virtual Machine. Holds for the first load after a clear cache, and for subsequent reloads as well.

Maybe running under a Virtual Machine "helps" reproducing...

[Eric Durocher]

I reproduce with the URL too, on Windows 7 (not a VM) / IE 10.0.9200.16438. The problem can also be seen in the mobile gallery demo (demos/mobileGallery), just clicking any item in the list.

[bill]

I see, OK you guys should backport the fix then since you can reproduce the error.

[Eric Durocher]

Backport for 1.7 including test case - Eric Durocher (IBM, CCLA), Adrian Vasiliu (IBM, CCLA)

[cjolif]

In [30293]:

refs #16425. Backport fix to 1.7 + test case. !strict.