[BUGFIX release] Prevent triggering V8 memory leak bug through registry / resolver access. #12666
Conversation
dgeb
force-pushed
the
disentangle-registry-hooks
branch
from
a1e2dc9
to
52bf722
Compare
| let resolved; | ||
|
|
||
| if (registry.resolver) { | ||
| if (typeof registry.resolver === 'function') { |
There was a problem hiding this comment.
Since registry.resolver is already taken care of in Registry's constructor, we should be able to remove this extra conditional. I do not believe that changing the resolver after registry creation is a thing we should support...
There was a problem hiding this comment.
Ok, I'll remove this check and the corresponding test.
|
Looks good other than that one comment about lazily assigning |
…ry / resolver access. This fix changes the expectations of Registry to accept a `resolver` that’s an object with a `resolve` method instead of a straight function. This allows us to avoid closing over access to a resolver object inside a function. It also allows us to avoid setting functions which shadow prototype functions unnecessarily. Setting `Registry#resolver` to a function is still allowed through a constructor option. The `resolver` function will be converted into an object with a `resolve` method and will result in a single deprecation warning. This fix also eliminates the need for application instances to override their registry’s `normalizeFullName` and `makeToString` methods. Instead, the fallback registry will be checked when evaluating these methods before returning the defaults. Again, this avoids the need to override instance functions that shadow prototype functions. [Fixes emberjs#12618]
dgeb
force-pushed
the
disentangle-registry-hooks
branch
from
52bf722
to
529bfc3
Compare
|
Ok, thanks for the review @rwjblue. I just eliminated the ability to lazily assign |
|
We still need confirmation from @stefanpenner that this fixes the leak he reported in #12618, but this is still a good refactor (and replaces some fairly gnarly code!) so I'm going to land this into canary + beta. Once confirmed RE: the leak (and that we don't introduce issues with this change into beta/canary) we can pull into release branch (currently 2.2.x). |
[BUGFIX release] Prevent triggering V8 memory leak bug through registry / resolver access.
|
I guess this requires changes to ember-test-helpers Would be happy to fix that if you guys can point me in the right direction - I'm not really sure where to start yet… |
|
@marcoow - We are tracking that issue over in ember-test-helpers emberjs/ember-test-helpers#126 and emberjs/ember-test-helpers#127. Hoping to get a fix out today... |
|
@rwjblue: great; let me know if I can help! |
|
I'm not sure if you prefer if I created an issue for it, but just to add another instance of the bug report for adopted-ember-addons/ember-metrics#55. For context, I worked through with @rwjblue on refactoring deprecated use of Tests are only failing in
|
original pr emberjs#12666 fixes emberjs#12618
This fix changes the expectations of Registry to accept a
resolverthat’s an object with a
resolvemethod instead of a straight function.This allows us to avoid closing over access to a resolver object inside
a function. It also allows us to avoid setting functions which shadow
prototype functions unnecessarily.
Setting
Registry#resolverto a function is still allowed butdeprecated, regardless of whether it’s through a constructor option or
via an instance override. The
resolverfunction will be converted intoan object with a
resolvemethod and will present a single deprecationwarning on first access.
This fix also eliminates the need for application instances to override
their registry’s
normalizeFullNameandmakeToStringmethods.Instead, the fallback registry will be checked when evaluating these
methods before returning the defaults. Again, this avoids the need to
override instance functions that shadow prototype functions.
[Fixes #12618]
/cc @rwjblue @stefanpenner