[BUGFIX beta] Prevents autotracking ArrayProxy creation #17834

pzuraq · 2019-04-02T00:09:29Z

This PR prevents an issue with immediate invalidation within the new
autotracking transaction. ArrayProxy currently reads from one of its
own properties, hasArrayObservers, and then immediately updates it
with notifyPropertyChange.

We avoid this by using a native descriptor instead of a computed, and
not using get() to access it. This way, nothing is tracked during
creation (even if it is mutated).

The original PR attempted to add array observers lazily, when the array was first accessed. However, tests in addons (such as Ember Data) were failing with this strategy. One issue was that adding observers to length wouldn't work, since there was no way to add the array observer when that occured. This strategy keeps the current semantics the same, and prevents the backtracking issue.

Also updated the PR to deprecate in unknownProperty if this occurs again, since it could happen in existing code today.

pzuraq · 2019-04-02T03:11:48Z

cc @wagenet, this should fix the bug we found

packages/@ember/-internals/runtime/lib/system/array_proxy.js

rwjblue

Are we certain that these two places are the only places that "consume" the array proxy? I haven't traced all the codepaths for the underlying mixins (MutableArray, ArrayMixin, etc)...

packages/@ember/-internals/runtime/lib/system/array_proxy.js

pzuraq · 2019-04-02T17:12:10Z

So the general contract for any EmberArray class is that all accesses should go through objectAt, which makes it the primary API for accessing any item in the array. Then length is the only other API that you can use to view the state of the array, and I think the fact that these are the two values that the array proxy keeps in sync confirms this.

Other than that, the one thing that requires us to add observers is adding an observer to the array proxy itself, since it then must start forwarding events. This is why the PR changes all uses to calling arr.addArrayObserver if it exists as well.

We do expose Ember.addArrayObserver, but I couldn't find a single usage of it on EmberObserver, and it's not documented. I think this PR effectively breaks array proxies if someone is using that API. We could make it a wrapper which calls arr.addArrayObserver if it exists, or we could drop it from being exported. I was mistaken, we don't actually expose Ember.addArrayObserver, I think this is good to go!

wagenet · 2019-04-02T19:06:03Z

I'm pretty sure our app hits the problem in areas other than ArrayProxy. I'm working on getting more details. @wycats also seemed to think it was a larger issue.

pzuraq · 2019-04-02T19:17:05Z

@wagenet yeah, it's definitely possible, if you have other cases we could definitely use them! I think lazily setting up the observers here is still a good change regardless, so our thinking was let's get this merged for now and keep finding more reproductions. If we find enough to suggest that this will be a major breaking issue, we'll have to rethink certain aspects of autotracking.

wagenet · 2019-04-02T20:10:58Z

I have verified that this does not fix all of the issues in my app. I'm investigating further.

rwjblue · 2019-04-02T21:07:00Z

I'm pretty sure our app hits the problem in areas other than ArrayProxy. I'm working on getting more details. @wycats also seemed to think it was a larger issue.

Yep, totally agree that this doesn't fix everything, but I think it's a good change regardless. Specifically, making ArrayProxy lazier on its own is a "nice to have"...

hjdivad

Also, i think @rwjblue mentioned this out of band as well, but we should fix Ember.addArrayObserver to delegate; having ArrayProxy not consume on addArrayObserver seems like a mega-troll for users who run into it. It may not be used in EmberObserver but I wouldn't want to bet on it not being used in apps.

packages/@ember/-internals/glimmer/tests/integration/components/tracked-test.js

pzuraq · 2019-04-03T01:58:56Z

I was actually mistaken about Ember.addArrayObserver existing, I saw it in a test that I assumed was for globals, but it was for documentation.

hjdivad

❤️ 👍

rwjblue · 2019-06-01T17:17:12Z

@pzuraq - I think this change is still something we want to land, even though it isn't required to fix the infinite rendering issues that originally caused us to look at this. Would you mind rebasing?

scottmessinger · 2019-06-05T16:35:24Z

It looks like I'm running into this same error using ObjectProxy.extend(PromiseProxyMixin). As soon as a return a ObjectPromiseProxy, things infinitely render. Any chance what I'm running into is related to this error?

pzuraq · 2019-06-05T20:19:24Z

@scottmessinger is this with the tracked properties feature flag turned on? And if so, does it work if the flag is disabled?

scottmessinger · 2019-06-05T20:22:06Z

@pzuraq I don't think so? How do I do that? I'm using the latest canary. Here's the features list I'm seeing: https://github.com/emberjs/ember.js/blob/master/FEATURES.md

scottmessinger · 2019-06-05T20:24:05Z

@pzuraq Yes! I do have it enabled -- sorry, took me a while to find it. Here's what I have:

        EMBER_NATIVE_DECORATOR_SUPPORT: true,
        EMBER_METAL_TRACKED_PROPERTIES: true,
        EMBER_GLIMMER_ANGLE_BRACKET_NESTED_LOOKUP: true,
        EMBER_GLIMMER_ANGLE_BRACKET_BUILT_INS: true,

scottmessinger · 2019-06-05T20:25:54Z

Also, the infinite rendering is happening if I return a promise from a getter and then pass it to the https://github.com/tildeio/ember-async-await-helper component. That library doesn't use ObjectProxy, so it's possible my issue is caused by something else.

pzuraq · 2019-06-05T20:30:28Z

For sure, if you could upload a reproduction that would be super helpful. This is definitely the kind of issue we're keeping our eyes out for since we're trying to land tracked properties relatively soon!

scottmessinger · 2019-06-05T20:31:30Z

Sure! What's the easiest way to do that these days? It doesn't look like ember twiddle is up to date. Would just a new ember app with octane be the best or is there anything faster?

pzuraq · 2019-06-05T20:32:54Z

Yup, especially for canary bugs since ember-twiddle has never supported canary/canary-flags (maybe we can do some work on that in the near future..)

An app repo would be perfect.

scottmessinger · 2019-06-05T22:24:03Z

@pzuraq Hopefully this will work: I made two failing test cases: scottmessinger@e349e96

It turns out the problem wasn't returning a PromiseProxy but returning null and then, later, a promise proxy. You might be wondering, "Why in the world would you ever do this?" Great question. In my app, I was finding this.args was undefined in the getter the first time it was was called. Later, they'd be populated. So, I just returned null if this.args was undefined. Later, when args was populated, I'd return the PromiseProxy or the promise.

Is that helpful?

scottmessinger · 2019-06-13T18:42:48Z

@pzuraq Let me know if you need anything else or the failing test case above isn't helpful!

pzuraq · 2019-06-13T18:44:21Z

Sorry, this is on my radar, I've just been super busy and haven't had a chance to dive in. I'll probably be able to get to this sometime next month

scottmessinger · 2019-06-13T19:33:21Z

@pzuraq No problem! Let me know if there's anything else I can do to be helpful!!

rwjblue

Can you prefix the commit with [BUGFIX beta]?

rwjblue · 2019-11-21T03:25:19Z

packages/@ember/-internals/runtime/lib/mixins/array.js

@@ -564,8 +565,10 @@ const ArrayMixin = Mixin.create(Enumerable, {
    @property {Boolean} hasArrayObservers
    @public
  */
-  hasArrayObservers: nonEnumerableComputed(function() {
-    return hasListeners(this, '@array:change') || hasListeners(this, '@array:before');
+  hasArrayObservers: descriptor({


Don’t we also need to add enumerable: false? Also, is caching important here?

Ah, yes, I meant to do that!

For caching, I think it depends on how many times we expect folks to run addArrayObserver and removeArrayObserver, since nothing else really uses it in the ecosystem or the codebase. It seems like that's also a relatively uncommon thing, but we could also keep it as a CP if we're worried and use untrack to access it and prevent the issue.

rwjblue

Can you add a test to packages/@ember/-internals/metal/tests/accessors/get_test.js for the "mutate inside unknownProperty" deprecation?

This PR prevents an issue with immediate invalidation within the new autotracking transaction. ArrayProxy currently reads from one of its own properties, `hasArrayObservers`, and then immediately updates it with `notifyPropertyChange`. We avoid this by using a native descriptor instead of a computed, and not using `get()` to access it. This way, nothing is tracked during creation (even if it is mutated).

rwjblue · 2019-11-22T15:15:17Z

packages/@ember/-internals/runtime/lib/system/array_proxy.js

@@ -111,11 +111,11 @@ export default class ArrayProxy extends EmberObject {
      this._arrangedContentRevision = value(this._arrangedContentTag);
    }

-    this._addArrangedContentArrayObsever();


LOL @ this typo, I stared at this line for ~ 3 minutes trying to figure out why it was in the git diff 😝

pzuraq requested review from rwjblue and krisselden April 2, 2019 00:09

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from 35c5287 to 3cfbf95 Compare April 2, 2019 00:15

chadhietala reviewed Apr 2, 2019

View reviewed changes

packages/@ember/-internals/runtime/lib/system/array_proxy.js Outdated Show resolved Hide resolved

rwjblue reviewed Apr 2, 2019

View reviewed changes

packages/@ember/-internals/runtime/lib/system/array_proxy.js Outdated Show resolved Hide resolved

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from 3cfbf95 to 6d3fd43 Compare April 2, 2019 17:05

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from 6d3fd43 to b9ec819 Compare April 2, 2019 17:16

rwjblue approved these changes Apr 2, 2019

View reviewed changes

hjdivad reviewed Apr 3, 2019

View reviewed changes

packages/@ember/-internals/glimmer/tests/integration/components/tracked-test.js Outdated Show resolved Hide resolved

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from b9ec819 to 0017138 Compare April 4, 2019 17:49

hjdivad approved these changes Apr 4, 2019

View reviewed changes

rwjblue added the Octane label Apr 11, 2019

krisselden approved these changes Jul 13, 2019

View reviewed changes

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from 0017138 to 1f67007 Compare November 21, 2019 02:52

pzuraq changed the title ~~[BUGFIX][Tracked Properties] Prevents autotracking ArrayProxy creation~~ [BUGFIX beta] Prevents autotracking ArrayProxy creation Nov 21, 2019

rwjblue reviewed Nov 21, 2019

View reviewed changes

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from 1f67007 to d9f2943 Compare November 21, 2019 04:40

rwjblue reviewed Nov 21, 2019

View reviewed changes

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from 2f9b2f9 to da96a73 Compare November 21, 2019 23:42

pzuraq force-pushed the bugfix/infinite-rerender/promise-array-proxy branch from da96a73 to 03c776b Compare November 22, 2019 01:33

rwjblue reviewed Nov 22, 2019

View reviewed changes

rwjblue approved these changes Nov 22, 2019

View reviewed changes

rwjblue merged commit bfeeb20 into master Nov 22, 2019

rwjblue deleted the bugfix/infinite-rerender/promise-array-proxy branch November 22, 2019 15:16

rwjblue mentioned this pull request Nov 22, 2019

chore: cleanup deprecations ember-m3/ember-m3#460

Merged

patocallaghan mentioned this pull request Feb 11, 2020

[Bug Ember 3.15.0+] Creating a tracked model with a fragment array throws back-tracking assertions adopted-ember-addons/ember-data-model-fragments#357

Closed

patocallaghan mentioned this pull request Feb 27, 2020

[Possible 3.15 regression] Model Fragment's fragmentArray throws backtracking rerender assertion from 3.15.x #18773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX beta] Prevents autotracking ArrayProxy creation #17834

[BUGFIX beta] Prevents autotracking ArrayProxy creation #17834

pzuraq commented Apr 2, 2019 •

edited

Loading

pzuraq commented Apr 2, 2019

rwjblue left a comment

pzuraq commented Apr 2, 2019 •

edited

Loading

wagenet commented Apr 2, 2019

pzuraq commented Apr 2, 2019

wagenet commented Apr 2, 2019

rwjblue commented Apr 2, 2019

hjdivad left a comment

pzuraq commented Apr 3, 2019

hjdivad left a comment

rwjblue commented Jun 1, 2019

scottmessinger commented Jun 5, 2019

pzuraq commented Jun 5, 2019 •

edited

Loading

scottmessinger commented Jun 5, 2019

scottmessinger commented Jun 5, 2019 •

edited

Loading

scottmessinger commented Jun 5, 2019

pzuraq commented Jun 5, 2019

scottmessinger commented Jun 5, 2019

pzuraq commented Jun 5, 2019

scottmessinger commented Jun 5, 2019

scottmessinger commented Jun 13, 2019

pzuraq commented Jun 13, 2019

scottmessinger commented Jun 13, 2019

rwjblue left a comment

rwjblue Nov 21, 2019

pzuraq Nov 21, 2019

rwjblue left a comment

rwjblue Nov 22, 2019

[BUGFIX beta] Prevents autotracking ArrayProxy creation #17834

[BUGFIX beta] Prevents autotracking ArrayProxy creation #17834

Conversation

pzuraq commented Apr 2, 2019 • edited Loading

pzuraq commented Apr 2, 2019

rwjblue left a comment

Choose a reason for hiding this comment

pzuraq commented Apr 2, 2019 • edited Loading

wagenet commented Apr 2, 2019

pzuraq commented Apr 2, 2019

wagenet commented Apr 2, 2019

rwjblue commented Apr 2, 2019

hjdivad left a comment

Choose a reason for hiding this comment

pzuraq commented Apr 3, 2019

hjdivad left a comment

Choose a reason for hiding this comment

rwjblue commented Jun 1, 2019

scottmessinger commented Jun 5, 2019

pzuraq commented Jun 5, 2019 • edited Loading

scottmessinger commented Jun 5, 2019

scottmessinger commented Jun 5, 2019 • edited Loading

scottmessinger commented Jun 5, 2019

pzuraq commented Jun 5, 2019

scottmessinger commented Jun 5, 2019

pzuraq commented Jun 5, 2019

scottmessinger commented Jun 5, 2019

scottmessinger commented Jun 13, 2019

pzuraq commented Jun 13, 2019

scottmessinger commented Jun 13, 2019

rwjblue left a comment

Choose a reason for hiding this comment

rwjblue Nov 21, 2019

Choose a reason for hiding this comment

pzuraq Nov 21, 2019

Choose a reason for hiding this comment

rwjblue left a comment

Choose a reason for hiding this comment

rwjblue Nov 22, 2019

Choose a reason for hiding this comment

pzuraq commented Apr 2, 2019 •

edited

Loading

pzuraq commented Apr 2, 2019 •

edited

Loading

pzuraq commented Jun 5, 2019 •

edited

Loading

scottmessinger commented Jun 5, 2019 •

edited

Loading