Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JavaScript Memory API #386

Closed
2 of 4 tasks
ulan opened this issue Jun 12, 2019 · 11 comments
Closed
2 of 4 tasks

JavaScript Memory API #386

ulan opened this issue Jun 12, 2019 · 11 comments
Assignees
Labels
Missing: spec Progress: review complete Resolution: satisfied The TAG is satisfied with this design Review type: CG early review An early review of general direction from a Community Group Topic: performance Venue: WICG

Comments

@ulan
Copy link

ulan commented Jun 12, 2019

こんにちはTAG!

I'm requesting a TAG review of:

Further details:

You should also know that...

JS code can figure out whether garbage collection happened between turns using this API. That is similar to JS WeakRefs.

The proposal was presented at WebPerf WG F2F June 2019 meeting.
Notes, slides, video are available here.

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our GitHub repo for each point of feedback
  • open a single issue in our GitHub repo for the entire review
  • [X ] leave review feedback as a comment in this issue and @-notify [@ulan]
@annevk
Copy link
Member

annevk commented Jun 26, 2019

From the slides I don't quite understand the same-origin security model given that in Chrome (aiui) same-site JavaScript objects might obtain irrevocable access to each other via document.domain. Is there a processing model available that has a more principled explanation as to when a security error would be returned?

@ulan
Copy link
Author

ulan commented Jun 26, 2019

@annevk "can access" should be interpreted according to the spec and independent from a particular implementation. Are you referring to the case when two iframes start with the same domain foo.bar.com and then one iframe changes its domain to bar.com while the other one stays with foo.bar.com?

If the API is invoked after the domain change, then the two iframes are considered as different origin.
In that case the implementation either throws a security error or guarantees that the objects accounted in the result can be accessed by the calling iframe according to the security checks defined in the spec.

There is a corner case when the domain of an iframe is changed after API invocation but before the result promise is resolved. Requiring the implementation to throw a security error in this case might be a safe bet here. I am not sure.

@annevk
Copy link
Member

annevk commented Jun 27, 2019

Well, or foo.bar.com and quuz.bar.com both using bar.com. How is the origin of an object determined? Are you relying on https://heycam.github.io/webidl/#dfn-associated-realm? (Despite some things building on that, it's not entirely clear to me how stable that is these days.)

@ulan
Copy link
Author

ulan commented Jun 27, 2019

Thanks for the link! That's indeed what I was relying on for JS objects. I knew that it is possible to find the realm of a JS object, but didn't know that spec already defines a term for it.

For other object types like strings, numbers, internal objects the implementation would have to infer the realms. Here are some ways to do it: https://github.com/ulan/javascript-agent-memory/blob/master/realm_attribution.md

I want to keep the mapping of objects to realms underspecified, so that different efficient implementations are possible. Some implementations might want to segregate the heap by realms, others might want to use some approximation of the retaining realm. If that's too expensive or infeasible, then throwing a security error is another option.

@annevk
Copy link
Member

annevk commented Jun 27, 2019

That sounds rather bad as then it would not be deterministic whether the method call throws or not and rather depend on implementation details.

@ulan
Copy link
Author

ulan commented Jun 27, 2019

Is it bad because developers might forget to add try/catch around the call and would be surprised by an exception in production?

If it is critical to avoid non-deterministic error throwing, we can move that part into the result. For example, the API could return an estimate range [0, max-heap-size] to signal that it cannot compute accurate result without leaking cross-origin information.

I want to mention that the result of the API is inherently implementation dependent and cannot be compared across browsers. That holds for any feasible memory measurement API.

@cynthia
Copy link
Member

cynthia commented Aug 1, 2019

@kenchris and I took a look at this today. From a quick review his looks useful, and we don't see any outstanding security issues or risks associated with this. The concern @annevk raises above is probably relevant, and given that making this deterministic is probably challenging, we'd at least like to see spec text (and example code) that suggests that things will be different across browsers, hopefully accompanied with examples that make it very clear that this can throw.

From the slides and explainer, it wasn't clear why this is cheap to collect - could you clarify a bit on that?

One last thing that came to mind was the usefulness of this since it only provides a summed value - for a complex SPA with lots of components that have been upgraded at the same time tracing down a leaky component would probably be rather difficult with just this. Would it be possible to consider optionally allow scoping the memory metrics? This most likely will not be as cheap to produce, but it feels like it would make the API a lot more useful.

@ulan
Copy link
Author

ulan commented Aug 1, 2019

Thanks a lot for taking a look!

I will make sure to emphasize in the text and examples that the results vary across browsers and that the function can throw.

The performance of the API depends on how a browser organizes JavaScript heaps. If a browser separates heaps by origin, then the API will be cheap. Otherwise, the browser has to do additional work to compute the memory usage. In any case, this is the cheapest memory measurement API we can get given the security constraint of not leaking information between origins. (That's the fundamental trade-off between performance and security).

Thanks for the suggestion about scoped memory metrics. I think they are solving much harder problem than this API does. They may be too expensive because they may require forcing garbage collection to be useful. Since the feasibility is not clear, I want to first focus on shipping the minimal API and consider scoped API as the next step.

@cynthia
Copy link
Member

cynthia commented Sep 10, 2019

Minimal API shipping sounds reasonable, we'd still appreciate if you could look into any of the points raised above if time allows. For the time being, we'll close this as we think this shouldn't block this moving forward.

Thanks for bringing this to our attention.

@cynthia cynthia added the Resolution: satisfied The TAG is satisfied with this design label Sep 10, 2019
@ulan
Copy link
Author

ulan commented Sep 11, 2019

Thank you @cynthia !

@ulan
Copy link
Author

ulan commented Feb 4, 2020

I updated the API to rely on COOP+COEP for security.

That greatly simplified the design and implementability of the API. It also resolves @annevk concern about non-deterministic exceptions. Now the API throws if (and only if) crossOriginIsolated == false.

That also allowed increasing the scope from a single JS agent to all JS agents of the web page (a big win for usability of the API).

The new version of the API is here: https://github.com/ulan/performance-measure-memory

@cynthia do I need start a new TAG review for the updated version of the API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing: spec Progress: review complete Resolution: satisfied The TAG is satisfied with this design Review type: CG early review An early review of general direction from a Community Group Topic: performance Venue: WICG
Projects
None yet
Development

No branches or pull requests

5 participants