WICG / web-app-launch

Web App Launch Handler
Other
74 stars 31 forks source link

A mechanism to avoid "bricked" app windows #43

Closed alancutter closed 1 year ago

alancutter commented 2 years ago

If a web app has

"launch_handler": {
  "route_to": "existing-client",
  "navigate_existing_client": "never"
}

in its manifest and one of its pages fails to call window.launchQueue.setConsumer() then the page will be a black hole all future app launches. This is a bit of a footgun, perhaps there is a different API design that could be more fail safe.

alancutter commented 2 years ago

Perhaps the design of LaunchQueue isn't appropriate and we should separate the initial LaunchParams from subsequent ones e.g.

// First launch appears here if applicable:
window.launchParams;

window.addEventListener('applaunch', event => {
  // Subsequent launches appear here:
  event.launchParams;

  // To disable the page navigation you have to:
  event.preventDefault();
});
alancutter commented 2 years ago

+@evanstade and @dmurph as this would affect file_handlers as well.

evanstade commented 2 years ago

By what point is window.launchParams guaranteed to be filled in?

Does this proposal mean navigate_existing_client no longer need exist, because that behavior is instead controlled by event.preventDefault() or lack thereof?

Could we instead change launchConsumer to return a bool (whether the launch was handled), and when there's no launchConsumer set assume it's not handled?

alancutter commented 2 years ago

By what point is window.launchParams guaranteed to be filled in?

window.launchParams can be a promise of the first LaunchParams.

Does this proposal mean navigate_existing_client no longer need exist, because that behavior is instead controlled by event.preventDefault() or lack thereof?

Yes, navigate_existing_client: never has a bit too much footgun potential.

Could we instead change launchConsumer to return a bool (whether the launch was handled), and when there's no launchConsumer set assume it's not handled?

This is a good idea for retaining the current API shape (or launchParams.preventDefault()) though I think the event.preventDefault() approach is more consistent with the rest of the web.

alancutter commented 2 years ago

So there are currently two API options here:

A. Split initial and subsequent launches up as window.launchParams promise and applaunch events (that can be preventDefault()ed) respectively. B. Keep window.launchQueue and add preventDefault() to the LaunchParams. Calling it for first launch can be a noop.

After discussing it with @mgiuca we agreed to go with B as A has the listener registration race condition with the event listener model. Though it is rare for the race to be triggered by a user it could happen via automated app launches or the user opening n files at once in an OS that launches the associated app n times all at once.

alancutter commented 2 years ago

I take back the comments about B avoiding race conditions. If you don't setConsumer() in time to preventDefault() the next launch then your page gets replaced. It's not really providing any benefit over the event model and kinds of voids the entire point of having this alternate LaunchQueue interface in the first place...

evanstade commented 2 years ago

So there are currently two API options here:

There's also (C), not making changes. There's no race condition if we keep navigate_existing_client rather than relying on preventDefault.

It's not really providing any benefit over the event model and kinds of voids the entire point of having this alternate LaunchQueue interface in the first place...

At a high level, I think it's nice for the initial and subsequent launches to work in the same way.

So compared to the status quo, the pros and cons of the options are:

(A) Pros - less footgunny, more idiomatic (at least for subsequent launches) Cons - introduces a race (for subsequent launches), initial and subsequent launches are handled differently

(B) Pros - less footgunny Cons - introduces a race (for subsequent launches)

How much do we worry about this footgun vs the race? I have noticed that in some cases, an OS will launch multiple files by executing the app with a single file on the command line, many times in succession, and some other times it will execute the app once with multiple files on the command line. In the former case, the app will launch a bunch of times in a short timespan, so we could in practice trip this race.

A footgun, on the other hand, is also possible even with (A), because you could write a handler function for applaunch that always calls preventDefault after a bunch of branching logic, where the branching logic fails to account for some case, and therefore does nothing, thus creating the black hole situation.

alancutter commented 2 years ago

I have noticed that in some cases, an OS will launch multiple files by executing the app with a single file on the command line

This sounds worthwhile to address as it's an idiomatic use case.

My worry about the link capturing black hole is mainly if a page fails to completely load due to network error rather than programmer error. I suppose in that case many other things won't be working either and the user will be inclined to refresh/restart the web app. Given we always focus an app window on launch the user's attention will be directed towards the thing that is at fault.

With these considerations it's probably better to avoid the race over the black hole. Ideally there's a design tweak that can mitigate the black hole without reintroducing the race condition but nothing comes to mind at the moment.

mgiuca commented 2 years ago

I agree with Evan's last comment. Discussed with Alan yesterday and today.

I prefer footguns over race conditions (prefer a thing you can get right to something you can't). Alan's concern is that this is more than a footgun, if there are network issues that means it's outside of the developer's control to get right.

Still, I think we can't design APIs around "what if the page doesn't load properly". That's an "all bets are off" situation. If we're concerned with that situation, then we should be thinking about this at the platform level (how can we make it so pages load "all or nothing" and don't have dangling resources), rather than trying to design APIs that are resilient to JS not loading. In my view, we should assume the page is working properly and avoid the race condition.

Side benefits of keeping things as they are:

alancutter commented 2 years ago

I think this is resolved for now, no changes to be made to spec.

alancutter commented 2 years ago

Reopening this one. I think this footgun is too easy to hit to let be.

I plan on adding a page to https://kitchen-sink-pwa.web.app to demo navigate_existing_client: never. To demo it on a single page I would have to add window.launchQueue?.setConsumer(({targetURL}) => location.href = targetURL) to every other page on the site. This feels extremely invasive and easy to mistakenly miss existing pages or pages added in the future. Additionally this black hole behaviour will occur if the user navigates to a non-HTML page e.g. app.com/image.png or app.com/diagram.svg or app.com/readme.txt where it's not possible to run JS.

alancutter commented 2 years ago

If the navigate_existing_client: never behaviour can be scoped then that will allow sites to reduce the blast radius of the footgun.

We could do this for navigate_existing_client:

"launch_handler": {
  "route_to": "existing-client",
  "navigate_existing_client": {
    "mode": "scoped",
    "scopes": ["/document"]
  }
}

Or we could do it more generally for launch_handler:

"launch_handlers": [{
  "scope": "/",
  "route_to": "existing-client",
}, {
  "scope": "/document",
  "route_to": "existing-client",
  "navigate_existing_client": "never"
}]
fallaciousreasoning commented 2 years ago

Random idea: What if we only considered a window to be an existing client it has a LaunchQueue consumer?

(maybe this brings us back to a race condition though...)

alancutter commented 2 years ago

Had a discussion with @g-ortuno about this.

I think my use case of having a single page using navigate_existing_client: never is esoteric enough to be handled by a service worker launch event instead of needing manifest support. Adding another type of scope is pretty undesirable conceptually and syntactically.

Some web apps will have non-app pages in their scope, usually because they're using their entire origin as their app scope but also host pages like documentation in the scope as well. These should be excluded from the app scope entirely, hopefully via an evolution of the scope_extensions proposal.

alancutter commented 2 years ago

Random idea: What if we only considered a window to be an existing client it has a LaunchQueue consumer?

(maybe this brings us back to a race condition though...)

Yeah that has the race condition again. We want to handle the user opening a list of files and the OS launching the app n times at the same time.

alancutter commented 2 years ago

Another option is to scrap navigate_existing_client, add LaunchParams.preventDefault() and have the default launch behaviour be:

await load event;
navigate page to launch URL;

So it's racy if your scripts take too long to register themselves but they still get a (probably) reasonable amount of time to do so.

alancutter commented 2 years ago

We could add an enable_prevent_default: true field to launch_handler so the above "await load event" app launch behaviour is an opt in. This way web apps that don't use preventDefault aren't penalised performance wise.

alancutter commented 2 years ago

Had some discussions on this with dominickn, the existing client should navigate if the document doesn't have a DOMWindow context (e.g. an SVG document) or is autogenerated by the user agent for a resource (e.g. viewing an XML file).