Record_array can infinite loop #608

Closed
wants to merge 60 commits into
from

Conversation

Projects
None yet
Contributor

endash commented Oct 1, 2011

There's a lot going on here, so I haven't dug too deeply into this, but in certain circumstances (I can very reliably cause this behaviour in my app just by calling createRecord() 4 times in a row. 3 times is fine, the 4th, not so much), #flush will loop infinitely on account of the 'changed' variable (line 509) existing, but being an empty set.

I'm preventing the loop by simply checking to see if changed has a length > 0 as well as existing.

lchambers and others added some commits Jun 9, 2011

lchambers
Fix a logic error that causes duplicate sourceKeys to be added if exp…
…andedRecordTypes returns more than 1 record type.
Tim Evans
experimental/menu_scroll: menu scroll view enhancements:
 * views were always scrollable- now when the the content fills
   the view scrolling is disabled.
 * 'autohidesVerticalScroller' will now autohide both scrollers
   at a time. in combination with 'autohidesVerticalScrollers',
   you can have the option of having both scrollers visible at
   all times, disappearing scrollers, or none at all.
toOne and toMany will now throw an exception if passed an undefined r…
…ecordType.

This can happen in real life due to a missing sc_require.
Jiří Zajpt
Fixed typo in SC.Query.build method.
Using multiple record types and conditions hash when calling
SC.Query.build method a wrong property name was used: recordsTypes
instead of recordType.
On iPad it is possible that the timer set in touchStart() will not ha…
…ve yet fired and that touchEnd() will come in in the same Run Loop as the one that started with the call to touchStart(). The bad thing that happens is that even though we cleared this.touch, occasionally if touchEnd() comes in right at the end of the Run Loop and then the timer expires and starts a new Run Loop to call beginTouchesInContent(), that this.touch will STILL exist here. There's no explanation for it, and it's not 100% reproducible, but there is about a 40ms window where the firing timer and the end touch cause a crash. What happens is that if we try to capture the touch that has already ended, assignTouch() in RootResponder will check touch.hasEnded and throw an exception.
Member

tim-evans commented on 90178cf Jul 26, 2011

Could this get patched over to the experimental scrolling stuff?

Member

tim-evans commented on 2df1c37 Jul 27, 2011

Thanks!

ColinCampbell and others added some commits Jul 29, 2011

Need to change location of attribute/classBinding additions again bec…
…ause of where Handlebars hooks into view rendering process
Fixed Text Field Support
"Change" event didn't fire on a text field preventing value from
updating during focus. Changed to "keyup" event to allow live value
updating without switching focus.
Public Keating
Merge pull request #529 from tim-evans/menu_scroll
SC.MenuScrollView and SC.MenuScrollerView refactor
Tests for nested scrolls (which work much better in this ScrollView (…
…after the following bug fixes) than in the default version)
The find() needed to be restricted to the child container view only, …
…otherwise it alters the styles of nested scroll views that were rendered earlier. Plus made the chained binding observe one less object, since parentView should never change for a scroller.
Added a new property called 'informLocation' for when you want to set…
… the location of the browser without triggering the actual routing. Makes the use of 'statecharts' and 'routing ' much easier.
Fixed a bug in the SparseArray API where a delegate will instantiate …
…a empty function for sparseArrayDidRequestIndexOf in the default case. This always falls back to looking locally
Merge pull request #551 from castaclip/master
corrected return values for SC.Record.isError() method
Merge pull request #548 from jzajpt/fix_sc_query_build_typo
Fixed typo in SC.Query.build method.
Merge pull request #536 from yappbox/fail_fast_on_undefined_record_type
Fail fast with a useful error when you pass toOne or toMany an undefined value
Merge pull request #501 from coolhand79/master
Fixed a simple logic error in record array
Member

tim-evans commented on 47c94a2 Aug 4, 2011

Just out of curiosity- did this fix the bug you were talking about?

Owner

publickeating replied Aug 4, 2011

Yessir, just didn’t have time to reply again to the email. Safari doesn’t support bind and a few others, like older IE and FF, don’t as well: according to http://kangax.github.com/es5-compat-table/

Member

tim-evans replied Aug 4, 2011

That's really weird... Thanks, though! :D

publickeating and others added some commits Aug 4, 2011

The touch deltas were always some crazy number, because they were sub…
…tracting the absolute start position from the relative current position. This meant that the ScrollView would always grab any dragging touches, even if they haven't actually passed the tolerance amount. This is particularly bad when you have nested ScrollViews, the inner ScrollView would never get a chance to scroll.
There is still a nasty bug on iPad related to touch events with SC.Sc…
…rollView that I'm determined to fix, but until then if we're going to throw an exception about it, we should at least spell correctly.
In SC.TextFieldSupport, update value and internal value separately to…
… ensure input value is not written unncessarily
Remove keyup handling from TextFieldSupport. Because it hooks into th…
…e responder chain, it is unnecessary to provide extra code paths
Merge pull request #593 from geojeff/master
Fix for skip settings in popover.css
Member

nicolasbadia commented on 1e572bb Sep 3, 2011

Since this fixes, if we edit a labelView using the mixin SC.InlineEditable, and we clic out of the field to commit the changes, SC throw: Maximum call stack size exceeded !

Jeff Pittman and others added some commits Sep 5, 2011

Owner

publickeating commented Dec 24, 2011

Hi. Are you able to add a unit test to this that will fail without your patch? We really need some way of reproducing this to know what's going on.

Owner

publickeating commented Mar 20, 2012

I'm going to close this for now. It's a simple change, but I'm worried about what deeper issue it might be hiding.

If you can create a unit test that fails without your patch, please open a new pull request and that would be greatly appreciated.

Thanks

Contributor

endash commented Mar 20, 2012

Wow somehow I missed all the action on this ticket. Sorry I never pulled a test together... I don't recall doing anything extraordinary, so I'm sure if there's a legit bug buried in this it'll shake out, if it hasn't already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment