fix: hide component to vm mapping from user #860
Conversation
ravijayaramappa
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I will like this to be a more generic mechanism. It seems that today, we use setInternalField and getInternalField methods. I will like to add a generic setHiddenInternalField and getHiddenInternalField, that should be analog to those other ones but using a WeakMap instead of own property, so we can use them across the board when needed.
|
Additionally, in IE11 we can continue using the non-hidden mechanism since we don't really care about locker, and we do care a lot about perf, WeakMaps in IE are slower. Also, all those assertions can be asserted on the originator of the call instead of the method itself. |
|
@caridy Having a common WeakMap to store all mappings we want to hide from the user will make the map size very big. The performance of WeakMap degrades as the size grows (See related issue https://bugs.chromium.org/p/v8/issues/detail?id=4086) |
|
@caridy I don't agree with |
|
@ravijayaramappa I'm not worried about weak map size. In fact, in IE11, if we have too many weak maps we will have memory leak problems because of the polyfill. |
|
I'm still unclear why we just dont patch |
| @@ -0,0 +1,23 @@ | |||
| import {setInternalField, getInternalField } from './fields'; | |||
|
|
|||
| const isCompatMode = 'getKey' in Proxy; | |||
There was a problem hiding this comment.
@caridy If there is a better way to detect compat mode, let me know
(locker is off in compat mode)
|
I'm wondering the same question as @diervo . Why can't we patch |
This comment has been minimized.
This comment has been minimized.
|
@diervo @davidturissini As long as the property is on the component, it is a liability. Patching |
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
@ravijayaramappa That makes no sense. We don't care what code execute when as long as we can guarantee our code is executed before creating the component which we can. Otherwise we will be in big trouble, not for this but for everything else. |
| @@ -0,0 +1,22 @@ | |||
| import {setInternalField, getInternalField, hasNativeSymbolsSupport } from './fields'; | |||
There was a problem hiding this comment.
these are just fields, keep them in shared/fields.ts
| ? (o: object, fieldName: symbol, value: any): void => { | ||
| let associationMap = associations[fieldName]; | ||
| if (!associationMap) { | ||
| associationMap = new WeakMap(); |
There was a problem hiding this comment.
this is not possible, we can't create weakmaps at will, all weakmaps in engine MUST BE globally defined at the module level.
My assumption here is that what you will do is to add the object as the key, and a hashtable for the value, and you can add new fieldNames into that value hashtable object to map.
|
@diervo @davidturissini getOwnPropertySymbols is not the only way to access those symbols, |
Storing the relationship directly on the component exposes it to end user @W-5628184
Benchmark resultsBase commit: lwc-engine-benchmark
|
| @@ -1,6 +1,7 @@ | |||
| import { compileTemplate } from 'test-utils'; | |||
|
|
|||
| import { createElement, LightningElement } from '../main'; | |||
| import { ViewModelReflection } from "../utils"; | |||
There was a problem hiding this comment.
We should not import engine internals in tests.
There was a problem hiding this comment.
yeah, I was thinking how to do this particular test in a way that we don't have to import this, but it is tricky beecasue it is computing the internal index.
There was a problem hiding this comment.
The test has been updated to fetch all symbols and verify that none them give access to the vm. Its a bit slower but asserts the same behavior.
We could also assert that there are not symbols on the instance.
expect(getOwnPropertySymbols(instance)).toEqual([]); Would you prefer a stricter assertion like that or sufficient to verify that none of the symbols give access to vm?
| * hiddenAssociations : (Component-A, { Symbol(ViewModel) : VM-1 }) | ||
| * | ||
| */ | ||
| const hiddenAssociations: WeakMap<object, object> = new WeakMap(); |
There was a problem hiding this comment.
| const hiddenAssociations: WeakMap<object, object> = new WeakMap(); | |
| const hiddenAssociations: WeakMap<any, any> = new WeakMap(); |
There was a problem hiding this comment.
I will recommend to to make this:
WeakMap<any, Record<symbol, any>>
| export const setHiddenAssociation = hasNativeSymbolsSupport | ||
| ? (o: object, fieldName: symbol, value: any): void => { | ||
| let associationByField = hiddenAssociations.get(o); | ||
| if (!associationByField) { |
There was a problem hiding this comment.
Use isUndefined helper method to avoid type cohersion.
| */ | ||
| const hiddenAssociations: WeakMap<object, object> = new WeakMap(); | ||
| export const setHiddenAssociation = hasNativeSymbolsSupport | ||
| ? (o: object, fieldName: symbol, value: any): void => { |
There was a problem hiding this comment.
| ? (o: object, fieldName: symbol, value: any): void => { | |
| ? (o: any, fieldName: symbol, value: any): void => { |
There was a problem hiding this comment.
@pmdartus o here is the WeakMap key. Its value is always going to be an object. And the method is not looking up any property on o. What would be the reason to type this parameter as any?
| associationByField = create(null); | ||
| hiddenAssociations.set(o, associationByField!); | ||
| } | ||
| associationByField![fieldName] = value; |
There was a problem hiding this comment.
At this point you should not need the ! notation.
| export const getHiddenAssociation = hasNativeSymbolsSupport | ||
| ? (o: object, fieldName: symbol): any => { | ||
| const associationByField = hiddenAssociations.get(o); | ||
| return associationByField && associationByField[fieldName]; |
There was a problem hiding this comment.
Use isUndefined helper function.
| assert.isTrue(vm && "cmpRoot" in vm, `${vm} is not a vm.`); | ||
| } | ||
| return getInternalField(component, ViewModelReflection) as VM; | ||
| return vm as VM; |
There was a problem hiding this comment.
the reason why this is duplicated is because in production we have a single liner function that browsers will inline. Making this a two lines to favor the debug portion is not needed, let's keep the one liner.
| let associationByField = hiddenAssociations.get(o); | ||
| if (!associationByField) { | ||
| associationByField = create(null); | ||
| hiddenAssociations.set(o, associationByField!); |
There was a problem hiding this comment.
! is not needed here, just make sure you use isUndefined() conditions as @pmdartus suggested.
| * | ||
| */ | ||
| const hiddenAssociations: WeakMap<object, object> = new WeakMap(); | ||
| export const setHiddenAssociation = hasNativeSymbolsSupport |
There was a problem hiding this comment.
I still think Association is the wrong word here, just setHiddenField should be fine.
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
|
||
| /** | ||
| * Store fields that should be hidden from outside world | ||
| * hiddenFieldsPerObject is a WeakMap. |
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
ravijayaramappa
changed the title
|
We have decided to merge this, and measure the impact in real world apps, in case it regresses. The initial assessment is that it will not have a significant impact. |
ravijayaramappa
deleted the
ravi/master/hideComponent2VMMapping/W-5628184
branch
Storing the relationship directly on the component exposes it to end user
@W-5628184
Details
Hide component to vm mapping from user. Accessing the VM allows user to internal state of the component and other vulnerabilities. W-5628184
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path for existing applications:
Please check if your PR fulfills the following requirements: