-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
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... |
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. |
Would it be reasonable to dispatch On the other hand, /cc @rakina who has been looking at similar things in navigation in the bfache context. |
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. |
I think we should keep firing |
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)? |
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. |
@domenic I've filed https://crbug.com/1124860 against Chromium's implementation to align with the proposed changes. |
Editors, any thoughts on merging this? It seems like something we should allow user agents to do. |
There was a problem hiding this 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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
It seems unnecessary and weird that we’d be firing |
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 |
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
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.