WICG / interventions

A place for browsers and web developers to collaborate on user agent interventions.
Other
177 stars 28 forks source link

A path for disabling sync XHR #62

Closed RByers closed 6 years ago

RByers commented 6 years ago

Synchronous XHR is bad for performance, and doing blocking network access on your UI thread is a long-known anti-pattern. Some thoughts / notes from TPAC discussion here

RByers commented 6 years ago

To summarize from the TPAC discussion, there was agreement between Chromium, Firefox and Edge that we should start with a Feature Policy that let's sites disable support for sync-XHR (causing it to fail similarly to a network failure). There are already major customers lined up (eg. AMP) looking to set this on some iframes they host to reduce their main thread jank.

@clelland is working on an implementation in chromium and will propose a concrete design here for discussion in a forum that can include Microsoft. Once there's rough consensus he'll propose a PR to the XHR spec.

/cc @annevk @cramforce

RByers commented 6 years ago

See discussion in https://github.com/whatwg/xhr/issues/178

RByers commented 6 years ago

Note the sync-xhr policy is shipping in Chrome 65+ to enable opt-out (but doesn't change any default behavior of course).

RByers commented 6 years ago

/cc @AngeloKai @spanicker

annevk commented 6 years ago

Why does this issue exist? Can't we discuss this between the XHR and Feature Policy repositories? It's not really an intervention, is it?

RByers commented 6 years ago

Tracking in the XHR repo makes sense to me (FP is really about the generic infrastructure, ideally not specific policies). We might someday want to do something more intervention-like (like flipping the policy default to disabled by default in some cases as part of a deprecation path), but we can always re-open this issue if it seems useful...

spanicker commented 6 years ago

Hey all, I am now working on disallowing sync xhr during beforunload and unload (page dismissal). Could you clarify if this is an "intervention" or a change that is now blessed by the spec?

The [spec](https://xhr.spec.whatwg.org/#the-open()-method) has a warning but it says that "UAs may experiment with throwing exception" rather than it's okay to throw exception in certain cases. Could the spec be re-worded to disallow sync xhr in certain cases like page dismissal? Thanks.

Full text of warning: "Synchronous XMLHttpRequest outside of workers is in the process of being removed from the web platform as it has detrimental effects to the end user’s experience. (This is a long process that takes many years.) Developers must not pass false for the async argument when current global object is a Window object. User agents are strongly encouraged to warn about such usage in developer tools and may experiment with throwing an "InvalidAccessError" DOMException when it occurs."

@RByers @annevk

annevk commented 6 years ago

@spanicker I think if such a restriction is meant to be adopted by other browsers it'd ideally be standardized via changes to HTML and XMLHttpRequest.

RByers commented 6 years ago

Exactly. "intervention" issues like this are intended to track experimentation and brainstorming. This is likely one of those cases other engines won't know how supportive to be of until we've proven it's sufficiently web compatible, so we might not be able to land the necessary change in HTML/XHR specs in advance of shipping in Chrome. But I think, like all interventions, we'd want to start with at least having some spec PRs ready before shipping, then circle back with data post-stable and try to get spec changes landed.

All that said, it wouldn't surprise me if @AngeloKai or @patrickkettner were to say that Edge had active interest in this and that they'd review and help us get agreement on spec text now.

spanicker commented 6 years ago

@AngeloKai and @patrickkettner - could you comment on interest from Edge on disallowing Sync XHR from beforeunload (& unload). There was agreement on this at TPAC per this doc: https://docs.google.com/document/d/1G4-oIXQBcqysk4PNHBzCE4XAaGURipPTbajfx5iDIRc/edit 3rd point under "Conclusion from TPAC discussion: Agreed we should all try to: ...", l listed as "Announce deprecation for unload/beforeunload case" Though I wasn't in the room so wanted to check if there is any further context here, or is it okay to go ahead with this now.

spanicker commented 6 years ago

@RByers @ojanvafai - were any other browser reps present for that? Could we cite the notes in that TPAC discussion section as vendor agreement on deprecation of sync xhr from beforeunload / unload?

RByers commented 6 years ago

There were folks from Firefox and Apple in the room, but we had network issues preventing us from taking detailed minutes so I don't want to risk speaking for anyone in particular. I definitely think it's safe to say that Microsoft supported that plan as I believe it was @AngeloKai who came to us with that doc suggesting deprecation the unload case first. That should be sufficient for our intent process, but feel free to point out that the notes say that there was agreement at the meeting conclusion to "Announce deprecation for unload/beforeunload case".