Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#16425 closed defect (fixed)

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

Reported by: Eric Durocher Owned by: Kris Zyp
Priority: blocker Milestone: 1.7.5
Component: Events Version: 1.8.1
Keywords: ie10 Cc: cjolif, Adrian Vasiliu
Blocked By: Blocking:

Description

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...
  1. 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.
  1. 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...

Attachments (3)

ie10-aspect.patch (639 bytes) - added by Eric Durocher 7 years ago.
Possible patch for IE10 / JIT problem - Eric Durocher (IBM, CCLA)
test_RoundRectList-dojo1.7.html (3.3 KB) - added by Adrian Vasiliu 7 years ago.
Test case for Dojo 1.7.
ie0-aspect-1.7.patch (4.3 KB) - added by Eric Durocher 7 years ago.
Backport for 1.7 including test case - Eric Durocher (IBM, CCLA), Adrian Vasiliu (IBM, CCLA)

Download all attachments as: .zip

Change History (26)

Changed 7 years ago by Eric Durocher

Attachment: ie10-aspect.patch added

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

comment:1 Changed 7 years ago by Eric Durocher

Cc: cjolif added

comment:2 Changed 7 years ago by cjolif

Keywords: ie10 added
Milestone: tbd1.8.2
Priority: undecidedblocker

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.

comment:3 Changed 7 years ago by cjolif

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

comment:4 Changed 7 years ago by 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.

comment:5 in reply to:  4 Changed 7 years ago by Eric Durocher

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

comment:6 Changed 7 years ago by Kris Zyp

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

comment:7 Changed 7 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

In [30133]:

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

comment:8 Changed 7 years ago by 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.

comment:9 Changed 7 years ago by 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.

comment:10 Changed 7 years ago by Eric Durocher

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

comment:11 Changed 7 years ago by Kris Zyp

In [30135]:

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

comment:12 Changed 7 years ago by Colin Snover

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

comment:13 Changed 7 years ago by Colin Snover

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

comment:14 Changed 7 years ago by 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).

comment:15 Changed 7 years ago by 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.

Last edited 7 years ago by bill (previous) (diff)

comment:16 Changed 7 years ago by cjolif

Cc: Adrian Vasiliu added

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

Changed 7 years ago by Adrian Vasiliu

Test case for Dojo 1.7.

comment:17 Changed 7 years ago by 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.

comment:19 Changed 7 years ago by 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...

comment:20 Changed 7 years ago by 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.

comment:21 Changed 7 years ago by bill

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

Changed 7 years ago by Eric Durocher

Attachment: ie0-aspect-1.7.patch added

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

comment:22 Changed 7 years ago by cjolif

In [30293]:

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

comment:23 Changed 5 years ago by bill

Milestone: 1.8.21.7.5
Note: See TracTickets for help on using tickets.