Resetting unrendered DataList attempts to access nonexistent boundsCache

Description

When passing an existing collection to a new DataList instance, a race condition can occur where the DataList tries to refresh itself before it's been rendered.

tries to find the (non-existent) list items that correspond with the deleted records by their index. During this search VerticalDelegate tries to calculate the amount of items contained within a single page, which triggers the call to enyo.DataList.delegates.vertical.width, which tries to retrieve the width from the boundsCache.

However, because the DataList has not been rendered yet, there are no bounds yet. The enyo.DataList.delegates.vertical.width does not anticipate this and causes an exception to be thrown.

Environment

Any

Activity

Show:
Ruben Vreeken
September 29, 2014, 11:46 AM

Created a pull request with a fix: https://github.com/enyojs/enyo/pull/868

Cole Davis (Enyo Team)
September 29, 2014, 6:28 PM

Hi , thank you for your submission here (and ). Can you please provide a simple example of how to recreate the issue you are seeing? I cannot get to the fail-case you are referencing in 2.4.0, 2.5.0 or master (2.5.1-pre.8). I also must be missing the race condition because there is guard code that does not allow the reset to continue should the list not have been rendered yet.

Again thank you for the submission and looking forward to seeing what I'm missing here.

Ruben Vreeken
September 30, 2014, 5:55 PM
Edited

So, I did some more digging around and it turns out this is not the race-condition I thought it was. There is a guard on the DataList that checks if it has been rendered before resetting it which works as intended. What does happen is the following:

I start out with a DataList and a Collection containing 6 records. The DataList is fully rendered and shows 6 rendered items.

Next, I resize my browser window to such an extend that my application decides to switch to a different form-factor (for instance from desktop to mobile).
When this switch is made, the following happens:

1) The existing DataList gets destroyed.
2) A new DataList gets instantiated. At this point, the collection is not yet attached to the new DataList.
3) The new DataList gets rendered without a collection and displays 0 records.
4) A binding attaches the collection to the new DataList after the empty list has already been rendered.

At this point we have the following state of affairs:

  • We have a fully rendered DataList containing 0 items.

  • We have a collection attached to the DataList containing 6 records.

5) Setting the new collection property on the DataList triggers an <propertyName>Changed callback. This callback orders the collection to fetchAndReplace.
6) The collection fetches, and removes it's old models.
7) This triggers the enyo.DataList.modelsRemoved method, leading to the following cascade:

  • enyo.DataList.modelsRemoved()

  • enyo.DataRepeater.modelsRemoved()

  • enyo.DataRepeater._select()

  • enyo.DataRepeater.getChildForIndex()

  • enyo.DataList.childForIndex()

  • enyo.DataList.delegates.vertical.childForIndex()

  • enyo.DataList.delegates.vertical.pageForIndex()

8) Once in arrived at enyo.DataList.delegates.vertical.pageForIndex(), list.controlsPerPage is still undefined, resulting in a call to enyo.DataList.delegates.vertical.controlsPerPage().
9) In enyo.DataList.delegates.vertical.controlsPerPage(), both list._updatedControlsPerPage and list._updatedBounds are still undefined.
10) enyo.DataList.delegates.vertical.controlsPerPage() executes the fn() call, executing it in the global scope but passing along the list.

Which leads us to the mix-up between this ticket and ENYO-4076:

At this point, list._updateBounds is undefined, so this.updateBounds(list) never gets executed. You pointed this out correctly in your comment on ENYO-4076, the missing context is not actually needed at that point.

However, list.boundsCache is also undefined at this point. So trying to access list.boundsCache.width or list.boundsCache.height at this point, results in the Error this pull-request attempts to fix.

The combination of both accessing a non-existent boundsCache and being in the wrong scope to try and generate a boundsCache on the spot looks like a bug or an unexpected case. I'm not sure if there will never be a case where calling this.updateBounds(list) when calculating the amount of controls per page would yield anything usable. If there isn't, I would argue it is still a bit confusing to have an unaccessible function-call, but that's would reduce to a matter of taste rather than a bug.

Cole Davis (Enyo Team)
September 30, 2014, 6:23 PM

Hi , thank you for the update.

First I wanted to clarify that for I was agreeing with you that it was an issue and that your fix should be incorporated. I apologize if I didn't make that clear. The other part of my comment was merely noting that since it hadn't come up as a bug before the majority case must not hit that path - clearly you were hitting it - I just wanted to understand better how/why you were hitting it and not just blindly incorporate a fix for a problem we don't thoroughly understand. The context is definitely missing and that was not intentional.

Next, it's clear from the API call fetchAndReplace that you're using 2.4 code as that method API was removed in 2.5. Interesting to note that there have been other changes with relation to the code-path you're hitting. In 2.5.1-pre the handling of the selected models is separate from the render/re-render path and will no longer cause the issue you're describing.

So, that also leaves us in an inconsistent place. I see two things here. If there is a bug here its in a previous release (2.4.x) and I don't foresee us doing another patch release for that series. A fix for the issue you're seeing won't apply to 2.5.1 and so I don't see that as an option. However, it does appear, from what you've described, that you're blindly executing fetchAndReplace on the collection change even when you don't need to. It should be relatively trivial to add a gate related to that handler to avoid unnecessarily doing that and thus avoid the issue you're facing altogether. And yes this seems like a race condition in the ordering of handlers once the collection has changed. If you overloaded a core method are you calling the super method after you do your custom work?

I just want to make sure to point out the reason we wouldn't fix this issue is because it is in an older release and it will be fixed in the upcoming 2.5.1 release. The good news is I believe you can very easily avoid the condition you're facing with a trivial test. I don't know the details of your application use-case (as to why it would need to call fetchAndReplace each time it was added to a different list) but if you can test for consistency or even the rendered state of the list - or simply wait until later in the tick to do the fetchAndReplace - it appears it should be ok.

Ruben Vreeken
September 30, 2014, 7:44 PM

Hi Cole,

Thanks for taking the time to look at this. Your reply certainly explains a lot. I am indeed working from an enyo 2.4 code-base and only did a quick check to see if the method itself had changed in 2.5 and (mistakenly) assumed the situation would be the same across both versions.

I think the race-condition issue originates from explicitly creating the DataList component without a collection and then relying on bindings to supply the collection to the list implicitly afterwards. Setting the collection at creation time and/or not blindly replacing the collection contents should keep the problem from occurring like you suggested.

Good to hear the problem is already no longer present in the upcoming 2.5.1 release. Feel free to close this issue.

Assignee

Screener (Enyo Team)

Reporter

Ruben Vreeken

Labels

None

External issue ID

None

Tango Test Run Elements

None

Old Issue Key

None

Components

Affects versions

Priority

None
Configure