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

Allow user agent to unload nested browsing contexts #5885

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

johnivdel
Copy link

@johnivdel johnivdel commented Sep 1, 2020

Adds a carve out which allows user agents to arbitrarily unload the nested browsing context of a browsing context container. This reuses existing steps for discarding a browsing context and loading user agent page inline when a navigation encounters a network error.

In the context of WICG/interventions#68, this allows the user agent to unload resource intensive ads to an error page, protecting users' devices.

There is not currently 2 implementer support for this intervention. This constraint was relaxed for other intervention related PRs such as #1400 when there was no opposition.

As this PR doesn't add any new behavior, but rather allows non-normative behavior, wpt are omitted.


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 8:00 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

Parsing MDN data...
Parsing...



If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@johnivdel johnivdel marked this pull request as ready for review September 1, 2020 19:48
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. @annevk do you know if this matches what Firefox does for its tracking protection? Maybe @hober could answer the similar question for Safari?

We might want to add an explanation of why the user agent would do this, although that could probably use some cross-vendor discussion to tweak the wording. The actual normative prose LGTM though.

@domenic
Copy link
Member

domenic commented Sep 1, 2020

Hmm, it occurs to me that using the top-level navigate means we will fire beforeunload, pagehide, and unload events at the frame that's being unloaded. Do you know if that's what Chromium does? (Other browsers, also curious about your approach!)

If the intent is to skip those, I wonder if perhaps you had it right in the first place, with jumping straight into "process a navigate response". I'd want to look a bit more closely though, if that's indeed the intent, before recommending either direction...

@johnivdel
Copy link
Author

The Chromium behavior is to skip the beforeunload event, but still fire pagehide and unload events. beforeunload is skipped so that the page may not try and prevent the UA from removing the problematic browsing context.

Firing unload is useful. For example if the top-level page is expecting a postMessage() from a nested context, firing unload allows there to be some communication that an error has occurred and the expected message will not be received.

@domenic
Copy link
Member

domenic commented Sep 2, 2020

Would it be reasonable to dispatch beforeunload, but not actually show the prompt? I think there's some precedent for that. I don't know of much precedent for firing unload without beforeunload.

On the other hand, beforeunload is pretty bad and deprecated, so if we want to omit it, I'm fine with that too; it'll just be a bit more work to wire up the spec. (Probably, we'd add a new optional named parameter like exceptionsEnabled, call it avoidPrompting, and then use that to guard navigation step 5.)

/cc @rakina who has been looking at similar things in navigation in the bfache context.

@johnivdel
Copy link
Author

I think that achieves the same goals.

One reason for not firing beforeunload, is that if beforeunload is fired, the #prompt-to-unload-a-document algo is completely disjoint from the #ua-unload-nested-browsing-context steps. While there are carve outs for the UA to not prompt, there is actually a causal relationship (at least in the context of WICG/interventions#68).

On the other hand, it's possible that there could be cases where the user agent would unload a browsing context and would want to allow a confirmation dialog.

@rakina
Copy link
Member

rakina commented Sep 3, 2020

I think we should keep firing beforeunload if we are firing unload events etc without the prompt. It's consistent with other behaviors in Chrome like tab closing (see code). It seems like prompting is already not required in prompt to unload steps and this seems like a reasonable case for the UA not to show the prompt?

@domenic
Copy link
Member

domenic commented Sep 3, 2020

Thanks for weighing in, @rakina! That pushes me toward that design, then. In that case, this spec PR is good. It might be worth trying to point out the causal relationship, e.g. adding a sentence after the "Navigate to..." one, but I'm not sure exactly how to phrase it.

@johnivdel, will it be OK to update Chromium to fire beforeunload (with no prompting)?

@annevk
Copy link
Member

annevk commented Sep 7, 2020

I don't think this matches any tracking protection mechanisms. Those rely on not fetching content. Not on rendering it and then deciding to toss it.

What comes to mind is dealing with crashes in an event loop and how a browser might deal with those. This should probably be equivalent to that in terms of observable behavior.

@johnivdel
Copy link
Author

@domenic I've filed https://crbug.com/1124860 against Chromium's implementation to align with the proposed changes.

@domenic
Copy link
Member

domenic commented Sep 23, 2020

Editors, any thoughts on merging this? It seems like something we should allow user agents to do.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence a bit whether this is good as it's not very clear to me how general this really is. Does Chrome use this for OOM or other types of crashes too?


<p>For a given <span data-x="bc-container">browsing context container</span> <var>container</var>,
the user agent can unload <var>container</var>'s <span>nested browsing context</span>
<var>browsingContext</var> for any reason by running the following steps:</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this wording can be improved. I don't think we should allow this for "any reason". It would also be better if it did not reuse "unload" where it means navigation.

@@ -77385,6 +77385,22 @@ console.assert(iframeWindow.frameElement === null);
</div>


<h5 id="ua-removal-nested-browsing-contexts">User agent removal of nested browsing contexts</h5>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems inaccurate as nothing is removed.

@rniwa
Copy link

rniwa commented Sep 24, 2020

It seems unnecessary and weird that we’d be firing beforeunload and unload in these cases. If a process that’s hosting an iframe gets killed, you wouldn’t get either anyway.

@domenic
Copy link
Member

domenic commented Sep 24, 2020

Right, I think there's a definite tension between OOO (where you don't want to, and in some architectures can't, run JS code) and unloading misbehaving sites like trackers/cryptominers/etc. For the latter, giving them a chance to clean up seems reasonable, and #5885 (comment) explains a case in which it would be useful.

@rniwa
Copy link

rniwa commented Sep 25, 2020

Right, I think there's a definite tension between OOO (where you don't want to, and in some architectures can't, run JS code) and unloading misbehaving sites like trackers/cryptominers/etc. For the latter, giving them a chance to clean up seems reasonable, and #5885 (comment) explains a case in which it would be useful.

I don't understand. If we're unloading a website because they're doing nefarius things, why do we want to give it an opportunity to run more scripts? That seems fundamentally contradictory approach. Like, if there was a malware running on your computer, the first thing you do isn't to tell it so that it can clean up after itself. We'd kill it and then cleanup whatever badness the malware left ourselves.

Also, when a process which is responsible for an iframe crashes, we can't fire unload / beforeunload events. This seems to imply that the spec would still have to allow this behavior even if we allowed events to be fired in such cases. I can tell you that if we ever implemented something like this, we'd likely never fire events given the two choices.

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

Successfully merging this pull request may close these issues.

5 participants