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

IFrame Execution Pausing #369

Closed
3 of 5 tasks
dtapuska opened this issue Apr 30, 2019 · 33 comments
Closed
3 of 5 tasks

IFrame Execution Pausing #369

dtapuska opened this issue Apr 30, 2019 · 33 comments

Comments

@dtapuska
Copy link

dtapuska commented Apr 30, 2019

Góðan dag TAG!

I'm requesting a TAG review of:

Further details (optional):

You should also know that...

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
  • leave review feedback as a comment in this issue and @-notify [github usernames]
@dtapuska
Copy link
Author

dtapuska commented May 1, 2019

I should mention the first tag review for page-lifecycle #283

@kenchris
Copy link

I really find the event names weird, the opposite of freeze is thaw and the opposite of resume is pause

@ylafon ylafon added Progress: untriaged Topic: scripting ECMA, Web Assembly bindings, etc. and removed Progress: unreviewed labels May 22, 2019
@kenchris
Copy link

@triblondon do you have comments on the policy feature naming used here?

@kenchris
Copy link

This description is very confusing "execution-while-out-of-viewport indicates freezing frames that aren't in the viewport" - execution != freezing - I assume that is because you intent it to be 'execution-while-out-of-viewport: none'

@dtapuska
Copy link
Author

Yes that is correct. Feature policies are what is allowed. And since the default allow list is '*' the policy is execution is allowed while outside the viewport and while not rendered. I previously proposed a freezing name but that was not desirable in terms of the properties of inheritence. Feedback that was received was that the policy should only apply to origins that we know are good; ie. if we want to allow execution of the example.com domain but nobody else. That is a desirable property of feature policy and using a freeze name removed that property. Likewise this makes sense in terms of matching other feature policies as well. ie fullscreen feature policy is set to "fullscreen: none" to deny access to fullscreen.

@kenchris
Copy link

kenchris commented May 22, 2019

So the terminology is not really interoperable: freezing, executing, resuming...

Discussing with @ylafon maybe something like run-outside-viewport, run-before-initially-displayed (or -visible) would be clearer?

or run-before-entering-viewport

@kenchris
Copy link

kenchris commented May 22, 2019

Actually, it needs to be clear whether the not rendered means not within viewport (yet - not initially rendered) or actually hidden. Looking at the definition, it seems to indicate hidden so lets call it something like run-while-hidden

@kenchris
Copy link

kenchris commented May 22, 2019

As this is on the Document, and you use the wording execution, why don't we use onscriptspaused and onscriptsresumed - it is very clear what what means

Plus execute-scripts-while-outside-viewport and execute-scripts-while-hidden

@dtapuska
Copy link
Author

Execution was chosen because it is a definiton that stems from ECMA 262, so that it clearly articules what is happening. Being rendered is a definition in the HTML spec and also has similar clearly defined behaviours.

Hidden is an overloaded term and implies a bunch of things that I don't think we want to cover. This is explicitly around not being rendered and only covers display: none.

execute-scripts-... implies you are just pausing script execution but you are in fact pausing the entire HTML loop which just isn't scripts.

@domenic @dvoytenko

@kenchris
Copy link

We definitely having naming issues, the team here had trouble understanding what it means without looking up terminology in specs and you cannot expect actual developers to ever do that.

@dtapuska
Copy link
Author

It took us a while to come up with these names and they were chosen to exactly articulate the behaviour that they convey. Basically you want "Allow Event Loop to run while the element does not intersect the viewport" and "Allow Event Loop to run while the element does not have an associated layout object".

@kenchris
Copy link

I could not deduct that from the names and don't believe normal developers will be able to either. Also I don't believe "rendered" is exposed in many DOM or CSS APIs at this point, but I might be wrong

@dtapuska
Copy link
Author

With the understood behavior we want in my comment above please focus on the design issues this feature might cause. Perhaps someone will come up with a marvelous alternate name in the meantime.

@kenchris
Copy link

@cynthia and me discussed this and think the idea is fine, but what do other browser vendors think of this?

We are a bit worried that this might just force ad vendors to pull the problematic logic out to the top-level browsing context if it affects their bottom line.

@kenchris
Copy link

@cynthia had that comment about what will happen if you do a while loop in say onfreeze - how long is that function allowed to run before script execution is paused?

@dtapuska
Copy link
Author

It will be frozen when it returns from the while loop; so possibly indefinitely. But fortunately doing something like an infinite loop on the main thread makes the entire page unusable since nothing will render or be processed. Typically browsers detect this and will end up marking the page as unresponsive and terminate it.

@cynthia
Copy link
Member

cynthia commented May 22, 2019

But shouldn’t those kind of cases (given that the frame is in a different process) be a case where a freeze would also be useful?

@dtapuska
Copy link
Author

dtapuska commented May 22, 2019

Sorry where in this do can you assume the frame is in a different process? I don't think I've made any assumption here in terms of cross origin behaviour, I explicitly call this out in the explainer.

A freeze is useful in both cases. But for the main thread we can't halt a thread that is in an infinite loop because there might be two windows that share the same agent and one is frozen and the other is not.

@dtapuska
Copy link
Author

dtapuska commented Sep 9, 2019

I'd like to proceed with Chrome shipping this but before I send such an intent to the blink mailing list I wanted to check back on this issue. There was some feedback around fixing feature policy first, that the current proposal seems reasonable but the names are somewhat difficult to interpret (inherit in feature policy's design). I'm not sure I'd like to wait for that to be resolved but I will pass that feedback along in my intent.

@dbaron
Copy link
Member

dbaron commented Sep 11, 2019

So @hober and I are looking at this during a breakout at the TAG meeting. We're trying to figure out what the state of prior discussions here is.

I think one issue raised was that freeze/resume is an odd pair of event names, rather than freeze/thaw or pause/resume, though that's not really an issue on this spec.

Another issue raised that's more directly tied to this is the comment:

@cynthia and me discussed this and think the idea is fine, but what do other browser vendors think of this?

We are a bit worried that this might just force ad vendors to pull the problematic logic out to the top-level browsing context if it affects their bottom line.

We're curious about both of these points: what's the level of interest from relevant folks at other browsers... and what about the concern that it seems like one of the major use cases for this might end up being worked around. In other words, will the embedding pages actually have not only the technical ability but also the market power to do this to their iframes when they want to do so?

@hober
Copy link
Contributor

hober commented Sep 11, 2019

Hi, @dbaron and I have been looking at this during a breakout in Tokyo today.

User agents have lots of information about the state of the device, and are able to intelligently pause or resume execution based on that information. Also, pausing and resuming execution is not "free", and may itself be a source of power etc. drain.

The page author has significantly less information than the user agent, and the controls provided here are necessarily more coarse as a result.

So I'm a bit worried that pages will engage in "cargo-cult" authoring behavior, and may complicate or interfere with the user agent's execution pausing logic. That said, if this usually results in significantly more execution pausing than the UA would have otherwise done, maybe it's a win.

@hober hober added Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed Progress: unreviewed labels Sep 11, 2019
@dbaron dbaron added Progress: unreviewed Venue: WICG Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review and removed Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review Progress: unreviewed labels Sep 11, 2019
@dtapuska
Copy link
Author

@hober I'm not seeing how this can interfer with the user-agent's execution pausing logic. Since it is impossible today to do what the explained in a web compatible way.

To clarify more are you saying Safari currently pauses iframes that are part of the active tab? I thought it only worked on entire pages that weren't active. Chrome does throttle iframes depending on certain conditions to improve battery life but we refer to that as throttling and not pausing. ie. running an event loop at a slower rate is different than not running any tasks at all.

I understand Apple has some uncertainity towards the page-lifecycle spec. Most I've heard is statements as you've made pausing and resuming aren't free. I'd like to understand what metrics Chrome could collect if we decide to ship this to help you give data to help convince you that we believe this will make the web better for the user.

I'd expect that any policy implemented by a user-agent would take precedence and this proposal is only about providing a more fine grained control over situations that the user agent can't reason about. ie; a user agent doesn't know that this display: none iframe shouldn't really do any work when it isn't displayed. We paused execution on these iframes accidentally in one version a few years ago and we got a large number of reports of the sites we broken. So we know very well that isn't interoperable with the web of today and we need some mechanism to describe these scenarios that the embedder wants.

I believe it will result in significantly more execution pausing than the UA can do. There is signifcant interest in these primitives

  1. Experimental code has already been written in AMP (Implement pausable iframes ampproject/amphtml#24110)
  2. There is from ad groups to improve battery life and user experience for pages.

@hober
Copy link
Contributor

hober commented Dec 5, 2019

  • Relevant time constraints or deadlines: M76 Chrome

It's unhelpful to state time constraints or deadlines in terms of a product's release cycle that reviewers may be unfamiliar with, or who may not know how to look up.

@dbaron
Copy link
Member

dbaron commented Dec 5, 2019

@hober @plinss and I are looking at this again at our Cupertino face-to-face meeting.

This seems like a reasonable feature to add to the Web, although as we've mentioned before we're not sure if it will actually be successful in the market.

One other technical concern that occurred to us this time that we didn't raise before is that this is building on top of feature policy, which is a spec that isn't yet adopted across browsers (and which I think some have concerns with). There's always a risk when building new features on top of other new features that the new feature on top might have been able to succeed on its own, but fails as a result of the failure of the feature underneath it. That doesn't necessarily mean it's better to do it a different way, but it might be worth thinking about. (It might also be worth taking some effort to improve cross-implementer consensus about feature policy given the desire to build a bunch of other things on top of it.)

@hober
Copy link
Contributor

hober commented Dec 5, 2019

I understand Apple has some uncertainity towards the page-lifecycle spec. Most I've heard is statements as you've made pausing and resuming aren't free. I'd like to understand what metrics Chrome could collect if we decide to ship this to help you give data to help convince you that we believe this will make the web better for the user.

I'll think about this and try to get back to you.

@dbaron dbaron added Progress: in progress and removed Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review labels Dec 5, 2019
@hober hober added this to the 2020-05-21-f2f-seoul milestone May 7, 2020
@dbaron
Copy link
Member

dbaron commented May 26, 2020

Looking at this again at our virtual face-to-face. One question to start:

Given that Feature Policy is now Permissions Policy, does the way this interacts with it fit with the current intent and design of Permissions Policy?

@dtapuska
Copy link
Author

Let's pause this review at this time. Being that this is dependent on #397 we should wait on that being resolved first.

@dbaron
Copy link
Member

dbaron commented May 26, 2020

Also, is it intentional that there's another copy of the spec in the iframe-freeze repo that seems pretty similar to the one you linked above. (Ideally, if it's obsolete, it would be great to remove it from the repo.)

@dbaron dbaron added the Progress: blocked on dependency Paused while some other design review finishes up. label May 26, 2020
@a2sheppy
Copy link

a2sheppy commented May 28, 2020

As one of the people who would need to help developers learn this, I will say it's important to choose names carefully, and to be sure that we don't choose names because it's what spec authors use, but because they're what web developers will understand. Instead of looking to specs when naming these events and the like, it would be a better idea to read developer documentation for similar features to see what terminology gets used "in the real world."

@hober
Copy link
Contributor

hober commented Sep 23, 2020

Let's pause this review at this time. Being that this is dependent on #397 we should wait on that being resolved first.

Since #397 is closed now, we should circle back to this. @dtapuska, has anything changed significantly since we last took a look?

@hober hober removed the Progress: blocked on dependency Paused while some other design review finishes up. label Sep 23, 2020
@dtapuska
Copy link
Author

We still wish to pursue this but the design is going to change. So going to close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants