Add SubjectMetadataController

[rekmarks]

The extension permission metadata functionality is currently housed within its local permissions controller. This PR extracts this functionality into a new "subject" (previously known as "domain") metadata controller, per the extension implementation.

[rekmarks]

We should merge this PR into main once the new permissions controller has been merged. Also TBD whether we're going to implement metamask_sendDomainMetadata as part of this PR or do something else with that.

Update: The permission controller has been merged, and we're not going to implement metamask_sendDomainMetadata here.

[Gudahtt]

So wait.... we're removing the origin from this list, even if we want to preserve the metadata? Why would we let these two data sets get out of sync like that?

[Gudahtt]

I see that the existing subject metadata controller works basically the same way, and it seems like a bug there as well.

[rekmarks]

No, this is intentional. If a subject encountered since startup has permissions, the current implementation essentially says "screw it, if permissions are ever removed for this subject, we'll let the next subsequent call to trimMetadataState remove it". That'll likely occur at the next reboot of the extension, and that should be good enough for our purposes.

[Gudahtt]

But why keep it around indefinitely if we don't need to? I don't see the advantage to doing this.

If we keep these two collections in sync, and only remove from the set when we remove the actual metadata, we can be confident that the trimming will work correctly and remove any excess metadata without associated permissions. As things stand now it could grow unbounded until the next reset.

[rekmarks]

As things stand now it could grow unbounded until the next reset.

Yes, but since it's been working like this for ~two years, we can be confident that it doesn't grow out of control in practice. The next reset will be no later than the next update, and our update frequency is if anything going to increase as opposed to decrease. As for users that don't update or restart their browsers, I think that's out of scope.

If if we were to implement your suggestion, unbounded growth is still theoretically possible since we never delete metadata for subjects with permissions under any circumstance.

But why keep it around indefinitely if we don't need to? I don't see the advantage to doing this.

There are advantages, though! In your suggested implementation, we would have to iterate over the entire since-startup set if it's larger than the cache limit, check the permissions of each subject, and delete the first one we find without permissions. The unlucky case here is that the user adds permissions to every subject they encounter. In that case, there will eventually be a computational cost to performing this iteration every time the user visits a new website.

In addition, changing how this work can result in a degraded user experience. Consider again the case where the user adds permissions to every subject under your suggested implementation. Let's say, after hitting the cache limit, they visit a new website. By the time we receive a permissions request from that website, its metadata will have been deleted. The only way to add metadata for it and any subsequent new websites would be to accept the request (without metadata) and refresh the page.

This brings me to the final advantage, which is that improving the existing implementation is more complicated than it seems, and probably not worth our time given that the existing implementation demonstrably works in practice. I think the real solution to this problem is to be able to request metadata on demand, which requires the series of changes to json-rpc-engine we've discussed in the past.

[Gudahtt]

Consider again the case where the user adds permissions to every subject under your suggested implementation. Let's say, after hitting the cache limit, they visit a new website. By the time we receive a permissions request from that website, its metadata will have been deleted. The only way to add metadata for it and any subsequent new websites would be to accept the request (without metadata) and refresh the page.

Ok, fair enough, that makes sense. I still don't like the idea of letting it grow unbounded, and I do not accept the last two years as evidence that this isn't a problem (that does not follow). But you are right that my suggestion is bad and this is more complicated than I thought.

Could we at least rename the variable though? I expected subjectsEncounteredSinceStartup to be the subjects encountered since startup, and was very surprised to find that it was not that at all. Maybe something like subjectsWithoutPermissionsEcounteredSinceStartup? It's absurdly long but it's clear at least. Or subjectsEncounteredWithoutPermissions, since "since startup" is implied as this can't be persisted.

[rekmarks]

Done in: d4cdb68

[Gudahtt]

LGTM!