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

Should generation of CropTarget from elements be able to fail? #48

Open
youennf opened this issue May 19, 2022 · 20 comments · May be fixed by #47
Open

Should generation of CropTarget from elements be able to fail? #48

youennf opened this issue May 19, 2022 · 20 comments · May be fixed by #47

Comments

@youennf
Copy link
Contributor

youennf commented May 19, 2022

As discussed in #17, it has been clarified that Chrome is currently failing CropTarget generation in some cases, in particular if a renderer process generates too many of them.

We should understand two things:

  1. Is it a good idea to expose such failures to web developers at CropTarget generation time? Current Chrome's implementation has privacy concerns.
  2. Are such failures something we should allow or is it an artefact of Chrome's current implementation that can be fixed?
@eladalon1983
Copy link
Member

eladalon1983 commented May 19, 2022

  • I think we should allow it to fail if the implementation does not yet support a specific Element type, for a start. Ideally all implementations will support all Element types. But we don't live in an ideal world.
  • It sounds quite reasonable to also allow failure for reasons of resource-limitations in the browser.
  • I think cropTo() should also be allowed to fail for resource-limitation, while we're at it.

@jan-ivar
Copy link
Member

Barring optimizations, the role of produceCropTarget is to create an association between an interface that cannot be serialized with one that can.¹ There is no reason for this to fail. If I busy-spin on creating these, resource exhaustion seems on par with every other trivial platform object, and we don't go about annotating these with failure paths.

1. "Calling produceCropTarget on an Element of a supported type associates that Element with a CropTarget. This CropTarget may be used as input to cropTo."

@youennf
Copy link
Contributor Author

youennf commented May 19, 2022

I agree with @jan-ivar here.

  • I think we should allow it to fail if the implementation does not yet support a specific Element type, for a start. Ideally all implementations will support all Element types. But we don't live in an ideal world.

It seems out of scope of this particular issue, WebIDL should solve this without the spec saying anything.

  • It sounds quite reasonable to also allow failure for reasons of resource-limitations in the browser.

Why? This is a pattern we do not see in any spec. As already said, this is a potential privacy issue as implemented in Chrome and I do not see why we should specify behaviours with such hazards.

  • I think cropTo() should also be allowed to fail for resource-limitation, while we're at it.

That sounds ok to me. cropTo is an expensive method, it already has an error code path, reusing it is not harmful.

@youennf youennf linked a pull request May 19, 2022 that will close this issue
@eladalon1983
Copy link
Member

eladalon1983 commented May 20, 2022

It seems out of scope of this particular issue, WebIDL should solve this without the spec saying anything.

  • If we standardize that Element should be received, and one browser accepts (IFrame or Div), that's an interop issue. Better document it in the spec.
  • Element has a lot of descendants. Suppose for the sake of argument that Chrome supports every single descendant except one. Do we now specify in the IDL that we support (ElementType1 or ElementType2 or ... ElementType2950)?

Why? This is a pattern we do not see in any spec. As already said, this is a potential privacy issue as implemented in Chrome and I do not see why we should specify behaviours with such hazards.

This is not identical, but it's similar:

The set of effective Capabilities may be platform dependent.
For example, on a resource-limited device it may not be possibl
to set properties P1 and P2 both to 'high', while on another
less limited device, this may be possible.

@youennf
Copy link
Contributor Author

youennf commented May 23, 2022

  • If we standardize that Element should be received, and one browser accepts (IFrame or Div)

Do we have such case? I would hope not.
In Chrome's case, my understanding is that the WebIDL might accept HTMLElement and not Element, this is fine and is handled by WebIDL just fine.

  • Do we now specify in the IDL that we support (ElementType1 or ElementType2 or ... ElementType2950)?

We could if we think this makes sense. Some API support BufferSource, some only ArrayBuffer.
If we have to go down there, we should probably rethink our API shape though.

This is not identical, but it's similar:

How is it similar?
There are hardware limitations which do not allow to support more than a maximum number of pixels, a UA must have to deal with these limitations.
In our case, failing a CropTarget is due to a UA optimisation to reduce latency. The UA could have decided to go with another approach.

@youennf
Copy link
Contributor Author

youennf commented May 23, 2022

Additionally, Chrome's implementation AIUI is failing CropTarget generation once a fixed number of generated CropTarget is reached. This allows observing GC.
We usually try to avoid designs that allow observing GC.

@eladalon1983
Copy link
Member

Do we have such case? I would hope not.

Sorry, I don't understand this question. What is it attached to?

In Chrome's case, my understanding is that the WebIDL might accept HTMLElement and not Element, this is fine and is handled by WebIDL just fine.

I think you're recalling a different discussion here which is not, IMHO, relevant to the current discussion. Chrome has never implemented HTMLElement OR Element. Chrome has so far only implemented cropping to HTMLDivElement and HTMLIFrameElement.

Implementing cropping to a given set of related Element subclasses, and ensuring it works correctly, is significant work. This work must precede updating the IDL files. We cannot expose untested functionality to unsuspecting Web applications.

Assume we invest this work in all current elements right out of the gate. Great. Now what happens when a new element type is introduced?

It is best if we allow gradual rolling out of features. Web developers clamoring to crop to a DIV now, should not be forced to wait until we implement HTMLSuperObscureElement. They'll do just fine wrapping their obscure element inside of HTMLDivElement or a similar work-around.

In our case, failing a CropTarget is due to a UA optimisation to reduce latency.

In this particular thread (#48), we're also discussing failure due to incomplete implementation. (This particular type of failure can should be synchronous.)

We usually try to avoid designs that allow observing GC.

Chrome might implement as only freeing up tokens when the (sub-)document is navigated, and not when things are garbaged-collected. At any rate, this is not part of the specification.

@youennf
Copy link
Contributor Author

youennf commented May 23, 2022

we're also discussing failure due to incomplete implementation. (This particular type of failure can should be synchronous.)

Incomplete implementations are not defined in the spec, you can implement it however you like.
Throwing an exception/rejecting a promise might be fine, using WebIDL or synchronous checks.
Maybe it might be easier for web developers to generate a valid CropTarget attached to the nearest parent element for which cropping is supported (div or body/iframe). When rolling out support of a new element type, the cropping will become more accurate.
Or failing at cropTo time by considering that this was not a valid CropTarget.

In any case, a TypeError synchronous failure is very different from the kind of failures #47 is trying to add to the spec.
The scope of this issue is about the latter: should we allow crop target production to fail in case of surface state propagation failure?

Chrome has so far only implemented cropping to HTMLDivElement and HTMLIFrameElement.

I am surprised to hear that.
I remember stating that kind of possibility in favour of putting this API in element and not mediaDevices as a convenient way to improve feature detection. At the time, I remember you saying this was out of scope since we were going with Element.

@eladalon1983
Copy link
Member

eladalon1983 commented May 23, 2022

Throwing an exception/rejecting a promise might be fine, using WebIDL or synchronous checks.

So let's go for it and have one less outstanding issue. If I send a PR that says that a synchronous error should be raised if the element is not of a type supported by the implementation, will you approve it? We can discuss other failures separately.

At the time, I remember you saying this was out of scope since we were going with Element.

I only have a vague memory of this discussion. The current implementation in Chrome, as of the time of this writing, is here - always has been HTMLDivElement and HTMLIFrameElement.

Long-term, I expect everyone to support any Element, and feature-discovery to be unimportant. But for the short-term, with only some Elements being supported by implementations, I think the spec should make an affordance. I hope that clarifies my position?

@jan-ivar
Copy link
Member

jan-ivar commented May 24, 2022

This work must precede updating the IDL files.

@eladalon1983 that's backwards. Specs inform implementation, not the other way around. It seems perfectly fine to standardize what we want at this stage, which is FPWD. @youennf suggested ways an imperfect implementation can handle this, or failing that it's not like WebIDL per se physically prevents an implementation from inventing is own validation on top (it violates the spec, but it's not a technical limitation).

In any case, I agree it is out of scope here, so please open a separate issue.

@eladalon1983
Copy link
Member

eladalon1983 commented May 24, 2022

Specs inform implementation, not the other way around.

It is useful for specifications to reflect abiding reality. This serves Web developers. They often read these specs too, and can regard them as authoritative.

As for "abiding":

  • It is likely that some Element subtypes will be rejected for the foreseeable future. (And time will tell if this is a pattern common to multiple UAs.)
  • It is also likely that occasionally, a new subtype will be introduced. Whichever UA-implementation team adds that subtype, might not have as a high priority to ensure cropping works for that new subtype. Note that this is likely for any UA, and not just Chrome.

In any case, I agree it is out of scope here, so please open a separate issue.

I think the two issues are highly correlated, and revolve around whether the spec should specify that, in reality, the user agent may fail the process, and that spec authors accept this reality.

@jan-ivar
Copy link
Member

It is useful for specifications to reflect abiding reality. This serves Web developers. Developers often read these specs too, and who often treat these specs as authoritative.

@eladalon1983 that's not how "authoritative" works. What you appear to be saying is Chrome's implementation is authoritative and the spec is not and should be updated to match. Am I misreading that?

I oppose this view. I think Chrome's implementation is buggy so I've filed crbug 1328836. Let's discuss there.

I think the two issues are highly correlated, and revolve around whether the spec should specify that, in reality, the user agent may fail the process, and that spec authors accept this reality.

No they're not related, unless we conflate "Should generation of CropTarget from elements be able to fail?" with input validation, which is already present. Even Chrome performs input validation:

    await navigator.mediaDevices.produceCropId("");
TypeError: Failed to execute 'produceCropId' on 'MediaDevices': The provided value
is not of type '(HTMLDivElement or HTMLIFrameElement)'.

But this is built into WebIDL bindings and happens synchronously. This issue is about whether generation of a valid target can fail.

@eladalon1983
Copy link
Member

eladalon1983 commented May 24, 2022

What you appear to be saying is Chrome's implementation is authoritative and the spec is not and should be updated to match. Am I misreading that?

You are indeed misreading that.

I oppose this view.

We have that in common.

I think Chrome's implementation is buggy

It will be "buggy" once shipped. Right now it's "incomplete", and that's not at all surprising, given how the spec continued to evolve past the point where the Origin Trial started - which is par for the course.

But this is built into WebIDL bindings and happens synchronously. This issue is about whether generation of a valid target can fail.

That the IDL bindings are going to make the implementation reject synchronously, is an implementation detail. The spec says that any Element should be accepted. But this is not necessarily going to happen with Chrome; not at every single given time point as new Elements are specced and implemented. I think it's very likely that Safari and Firefox will behave similarly. Specs are read by both implementers and developers. It's good if it alerts developers to risks such as partial implementations.

@youennf
Copy link
Contributor Author

youennf commented May 24, 2022

It's good if it alerts developers to risks such as partial implementations.

I do not know any spec that does this. I do not see what is so special that would require doing something like this here.
MDN does this, maybe this is the place that you are looking for.

If Chrome is shipping a partial implementation, it is important to think of how to complete the implementation in following updates. Implementors do that exercise regularly when implementing specs without requesting spec changes.

If the issue is about supporting element is hard and might not be feasible in some environments, let's discuss this in a separate issue.

What we are discussing here is related to some of the changes being proposed in #47. Let's focus on this.

@eladalon1983
Copy link
Member

eladalon1983 commented May 24, 2022

In an out-of-band chat with Harald, he has raised the interesting point of HTMLAudioElement and similar elements. In other words, some elements simply do not make sense as crop-targets. That means we now have three loosely related issues:

  1. Elements like HTMLAudioElement, which do not make sense as crop-targets. [Edit: This is covered in What should the spec say about elements like HTMLAudioElement? #51.]
  2. Elements which do make sense as crop-targets, but which are unsupported by a given implementation at a given time. [Edit: This is covered in Need for a predictable error type for unimplemented Element subtypes #55.]
  3. Elements which do make sense as crop-targets, and which the implementation generally supports, but which it cannot support at a given point due to some limitation, e.g. constrained resources.

We can discuss these either separately or altogether.

@youennf
Copy link
Contributor Author

youennf commented May 24, 2022

In other words, some elements simply do not make sense as crop-targets.

HTMLAudioElement makes sense to me, but let's move this discussion elsewhere if you wan to have it.

All elements have a bounding box afaik so we are good in terms of spec definition after #20.
Some bounding boxes will probably be more used than others, that is fine.

We can discuss these either separately or altogether.

This is clearly a separate discussion, please file a separate issue.

@jan-ivar
Copy link
Member

Creating a {} crop target should not fail, because it would be premature to allocate cropping resources at this point for anything other than as a UA optimization. Any page can create cropTargets at any point, with zero knowledge of whether they'll ever be captured, making for a highly unreliable signal that cropping is about to happen.

UA optimizations are encouraged, but carry inherent complexity. I sense a general understanding in the web platform that:

  1. good optimizations focus their efforts on typical cases, as not every corner case needs to be optimized
  2. web developers should be shielded from complexity of when and how optimizations happen (if they happen)
  3. We don't spec optimizations. As long as an API doesn't limit it, the burden is on UAs to optimize within the API

@eladalon1983
Copy link
Member

Creating a {} crop target should not fail, because it would be premature to allocate cropping resources at this point

Regardless of whether anything is allocated, it is usually good to fail earlier rather than later, and for the failure to happen close to where things went off-track. Failing in cropTo() means failing in a different document, maybe even another tab one day [*]. When the failure is really associated with cropTo(), that's all well and good. But for failures of the type described in bulletpoint 2 of this message, there is no real association with cropTo(), and that allows failing earlier.

--
[*] See #63.

@jan-ivar
Copy link
Member

We've already agreed to discuss bulletpoint 2 in #55 not here, so let's stick to that. Input type validation is irrelevant here. This issue is about failing generation due to resource allocation, as described in PR #47, from which this issue was opened.

Creating a {} crop target should not fail, because it would be premature to allocate cropping resources at this point

Allowing random JS in would-be-captured documents to exhaust cropping resources seems highly problematic:

  • It's action at a distance, allowing JS libraries unrelated to cropping to DoS attack cropping without user permission
  • Defeating cropping may expose user information in unsuspecting poorly-written apps, creating a privacy footgun
  • Resource allocation this early is inherently unnecessary, a gamble, a premature optimization to avoid IPC in cropTo

DoS is easily avoided by simply doing IPC and resource allocation in cropTo. With that baseline, any earlier resource allocation is purely UA optimization, whose cost and complexity should be contained to said UA, within the existing API.

@youennf
Copy link
Contributor Author

youennf commented Jun 14, 2022

In addition to DoS, it might be a potential fingerprinting issue, implementations should really avoid these issues.
We should be careful to not introduce such potential pitfalls in specs.
Failing later at cropTo time has very limited privacy concerns given user gave permission to do capture.

Also, as discussed before, there are ways to avoid these issues while retaining the benefits of early allocation in non pathological cases. I do not see what prevents Chrome to switch to this kind of implementation.

I also disagree with failing earlier being better. I believe web developers will not account for this and this might lead to very mysterious reasons/hard to debug reasons.

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

Successfully merging a pull request may close this issue.

3 participants