fix(jqLite): Check "length" in obj in isArrayLike for iOS JIT bug #11508
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits. Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name. |
|
CLAs look good, thanks! |
|
Updated pull request to follow updated isArrayLike solution in jQuery jquery/jquery#2185 Unfortunately no tests since this is iOS8 only and doesn't surface in iOS simulator. |
|
@snapwich Could you add a comment explaining why it's needed? Otherwise someone will probably try to remove it in a couple of months or years ;) |
|
@realityking sure. I just added a comment similar to the jQuery commit. |
… JIT bug from surfacing
snapwich
changed the title
|
Any update on merging this? This fix has made its way into jQuery now and would be a quick fix for Angular. |
|
LGTM |
|
I believe jQuery found that based on their usage they didn't need the |
|
@jdalton that works in jQuery's case because the caller knows that it's a non-null "object" already, I'm not sure the same assumptions fly here. It would be good to make those assumptions work but I won't consider that a blocker |
|
@jdalton @caitp yeah jQuery and Angular make some different assumptions I think. Removing the Object(...) causes these unit tests to fail that otherwise don't: |
|
For reference jquery/jquery#2145 |
|
@mzgol - you worked on this bug in JQuery - what is your recommendation here? |
|
We should work around it; relevant jQuery commit: jquery/jquery@1541664 |
|
This looks good but in jQuery it turned out some people relied on undocumented behavior that jQuery.map worked on strings and not just arrays (this broke a popular DataTables jQuery plugin, they've already fixed it so we didn't touch the final implementation). Not sure if this is an issue here. |
|
OK, LGTM. Let's merge this. |
… JIT bug from surfacing Closes #11508
… JIT bug from surfacing Closes angular#11508
to prevent iOS8 JIT bug (https://bugs.webkit.org/show_bug.cgi?id=142792) from surfacing
This is the fix implemented in underscore.js and most likely the fix that will be implemented in jQuery.
It's not a very easy to recreate bug, but I stumbled upon it myself in our production app and was able to narrow it down to isArrayLike after several hours of banging my head. This plunkr was the closest I got to consistently surfacing the bug in Angular: http://plnkr.co/edit/UQ5NhZnRuafNxWiJuuyN
That ng-repeat will fail, but only on iOS8 (and i've personally only had it fail on iPad) about 50% of the time after a few digests. If it doesn't fail quick reloading the plunkr a few times will usually get it to fail.