Issue with `mergeMixins` when accessing a service in a getter

[nlfurniss]

Recently found an issue with mergeMixins. it affects Ember versions at least as far back as 3.12 and also current release and LTS.

As I understand it, the issue is when a mixin defines a property (doesn’t necessarily have a value) then we try to access the value during construction before owner is set. This breaks getters that access services. Shared findings with @pzuraq.

Error message: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container. Error: Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.

Code where issue arrises (Please note: the original link was to master 😞. I've changed the tag to the version of ember published the day before, but I have no memory of the original code it linked to)

Reproduction (Gist)

[boris-petrov]

I think this is somewhat related to https://github.com/emberjs/ember.js/issues/18829 - while debugging I was seeing the problem in that other issue on exactly the same line. @pzuraq can probably say if they are the same or different issues.

[chriskrycho]

@nlfurniss and I spent some time today debugging this and identified the root cause of this bug.

While recursing through mixins in mergeMixins(), we call mergeProps(). When it finds a property which doesn't have a descriptor from Ember's Meta, it simply assigns the value from the base class to the class it is merging with:

https://github.com/emberjs/ember.js/blob/55ffe6326f11efcaeb278cdf7e0f86543daa9f04/packages/%40ember/-internals/metal/lib/mixin.ts#L304-L311

However, this only works as designed if the type is a classic CP or observer or a plain value, not when it is a getter. This is because the simple assignment used here—

let prev = (values[key] = base[key]);

—by definition must evaluate base[key]. For the case where it is a classic computed property or observer, that's fine: it's just a reference. For the cases where it's a value type, that's fine: it just gets copied. For the cases where it's a reference type, that's fine: it's just a reference (the CP/observer case is actually a special case of this). For the case where it is a getter, it is not (necessarily) fine.

I emphasize “as designed” above because in many cases, it still happens to work… but if the getter in question happens to call a service injection and that happens during initialization such that the class in question hasn’t yet been associated with an owner, then the bug reported here appears. Calling the getter like this invokes it, which triggers the entire process of resolving an injection, which in turn drops down through to getOwner() on the object, and it may not yet be set up.

Evaluating the getter is not the intended behavior here at all, though. Instead of being evaluated, a getter needs to be "copied over", just as with CPs/observers/other items. (This particular code path is only a few years old, but from what I can tell from spelunking, I don’t think we ever handled this case quite correctly, unless perhaps by accident!) I suspect the solution will involve doing something roughly like this:

if (desc === undefined) {
  let baseObjDesc = Object.getOwnPropertyDescriptors(base);
  let nativeDesc = typeof baseObjDesc[key];
  if (nativeDesc?.get === 'function') {
    Object.defineProperty(values, key, nativeDesc);
  } else {
    let prev = (values[key] = base[key]);

    if (typeof prev === 'function') {
      updateObserversAndListeners(base, key, prev, false);
    }
  }
}

Additional considerations:

• How does this interop with the way we walk the prototype chain? Do we need to do the same thing with Object.getPrototypeOf(base) and Object.getPrototypeOf(values)? Or something else?
• Do we need to do the same thing when the descriptor has a set on it? Presumably!
• While I showed this locally for clarity, we definitely don’t want to do the Object.getOwnPropertyDescriptors(base) check here: it’s in the middle of a loop. It should be pushed up to the top of the function body instead.

[chriskrycho]

@rwjblue and I poked at this this morning and my basic read was correct. (PHEW) I'm working on getting a failing test up, hopefully by EOD today, and then will try to get up a PR which fixes this and maybe also improves the perf of this path if possible.

Notably, we try to do the right thing here… but it only handles the cases where we have an actual Mixin class rather than when merging POJOs.

[chriskrycho]

Additional debugging with @rwjblue after trying and failing to get a simple test case in place showed why this has been rare and helped us ID the root of the issue.

Our handling of getters and setters works correctly for cases where we have only classic classes, or only native classes, or native classes extending classic classes. In some cases, however, it fails when we have "zebra-striping" of class types, with a classic class extending a native class which extends a (stack of) classic class(es). We correctly handle native getters in mixin and POJOs passed to .extend(), but—quite reasonably—do not handle getters on native classes when calling .extend(SomeNativeClass). The result is that those getters are not decorated the way they are in mixins, so mixin merging ends up invoking the getter rather than defining a decorator which returns it.

[boris-petrov]

@chriskrycho I'm not sure it's the same issue (as I mentioned above) but there is something like a test case in the other issue. You can perhaps also use that.