1386711 - Consider making Sequence inherit from nsTArray, not FallibleTArray

Status: NEW

(Core :: DOM: Bindings (WebIDL), enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

The history here is that FallibleTArray and InfallibleTArray used to bake the allocator into the class type.  There was no way to do fallible operations with an InfallibleTArray.

Because Sequence is used for both in parameters and return values in the DOM, and in parameters _must_ be fallible (web content trivially controls the allocation size: "var arr = []; arr.length = 1e9;"), Sequence inherited from FallibleTArray.

But for return values, where _we_ may control the allocation size, it may be reasonable to use infallible array bits if we know the array is small.  As long as in parameters (which are handled by bindings code anyway) consistently use fallible operations, that end of things is ok.

There may be some value in sticking with FallibleTArray as the superclass to force people to consider whether their allocations might fail when returning sequences to script.  But I'm not quite sure how much value that is.

Any thoughts?

Comment 1

[Nathan Froyd [:froydnj]]

Sequence's use of FallibleTArray is one of, if not the, last big hurdle(s) for removing FallibleTArray entirely.  I think I'm in favor of switching it over to InfallibleTArray.

Bindings return parameters could be annotated with some static-analysis attribute that would require fallible operations to be used?  I guess that runs into some of the same problems we have with the current setup, and you'd have to come up with a scheme for handling Length() and similarly innocuous functions.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I think I'm in favor of switching it over to InfallibleTArray.

You mean nsTArray?  Why would we switch it to InfallibleTArray rather than nsTArray?

> Bindings return parameters could be annotated with some static-analysis attribute that would require fallible operations to be used?

I'm not sure it's worth it.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #2)
> > I think I'm in favor of switching it over to InfallibleTArray.
> 
> You mean nsTArray?  Why would we switch it to InfallibleTArray rather than
> nsTArray?

Yes, nsTArray, my mistake!  The InfallibleTArray alias would be going away anyway at some point.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Work in progress

This makes the switch, but the other fixups needed point out a behavior change from this switch: operator= on Sequence switches from fallible to infallible, as does the copy constructor.  Pretty sure this is not desirable because we end up using those for web input, so we need to think about this a bit.

Comment 5

[Boris Zbarsky [:bzbarsky]]

On the other hand, having silently-fallible operator= is not great either.  :(

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> This makes the switch, but the other fixups needed point out a behavior
> change from this switch: operator= on Sequence switches from fallible to
> infallible, as does the copy constructor.  Pretty sure this is not desirable
> because we end up using those for web input, so we need to think about this
> a bit.

Could we not explicitly delete these operators/constructors/methods on Sequence, and require people to call the fallible methods when they need it?

Comment 7

[Andrew Overholt [:overholt]]

Not sure if this should be P1 ...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Could we not explicitly delete these operators/constructors/methods on Sequence

We could, but we use them right now, including from autogenerated code (e.g. copy constructors and copy assignment operators for unions use them on sequence union members, same thing for dictionaries).

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > Could we not explicitly delete these operators/constructors/methods on Sequence
> 
> We could, but we use them right now, including from autogenerated code (e.g.
> copy constructors and copy assignment operators for unions use them on
> sequence union members, same thing for dictionaries).

So in the old code what would have happened for the copy constructor if we tried to copy an excessively large sequence would be that the new sequence would be empty, while in the new code we would MOZ_CRASH?

If we want we should be able to provide new constructors which do the "right thing", but it might not be worth it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Right, the old code would give an empty sequence.

This does not make me at all happy, mind you.

I'm going to see whether I can change bindings to not output copy construction stuff when sequences are involved and what code that breaks.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Right, the old code would give an empty sequence.
> 
> This does not make me at all happy, mind you.
> 
> I'm going to see whether I can change bindings to not output copy
> construction stuff when sequences are involved and what code that breaks.

Yeah, I think the old behavior is very unintuitive. If we want to remove implicit infallible behavior I think we need to have a better way to detect and handle failures than that ^.^.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So the things that breaks are:

1) VRDisplay::requestPresent.

This is handed a sequence<VRLayer> in the IDL, and VRLayer has leftBounds/rightBounds sequences.  requestPresent calls VRDisplayClient::BeginPresentation which calls VRDisplayPresentation::VRDisplayPresentation, which makes a copy of the input sequence into mDOMLayers.  That copy is infallible (and hence is a somewhat easy to trigger crash on 32-bit), but the copies of the sequences inside the dictionaries used to be fallible.

It's not quite clear what the right thing is to do here, short of having a fallible CloneTo on such dictionaries or something.  And maybe adding something on nsTArray to make fallible assign not just copy-construct the array elements?

2) VRDisplayPresentation::GetDOMLayers -- similar deal, but going in the opposite direction; this implements VRDisplay::GetLayers which is in webidl as "sequence<VRLayer> getLayers();".

3) KeyframeEffectReadOnly::GetProperties -- this is implementing webidl "sequence<AnimationPropertyDetails> getProperties()", where AnimationPropertyDetails is a dictionary with a "sequence<AnimationPropertyValueDetails> values" in it.  This method creates the dictionaries on the stack and appends them one by one; for this use case having a move constructor on the dictionary that calls the move constructor on the array would be good enough, and we can certainly do an infallible move constructor here, because that doesn't need new allocation.

4) MediaKeySystemAccess constructor, which is passed a MediaKeySystemConfiguration dictionary, which includes sequences.  This is much like the VRLayer situation.

5) MediaKeySystemAccess::GetConfiguration: same deal in reverse.  

6) GetSupporteConfig stuff in MediaKeySystemAccess.  Again, MediaKeySystemConfiguration& as outparam, but this case could use a move operator=.

7) MediaKeys constructor, which is passed MediaKeySystemConfiguration.

Actually, there's tons more stuff in dom/media (all sorts of Media*Constraints, etc).  There's also something trying to copy-construct an nsTArray<MediaKeySystemConfiguration> but the compiler won't tell me where something is: just talks about template instantiations, but not who requested them.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, and the compiler not telling me what it's complaining about makes it hard to get past this to see what other areas might have problems with this.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch I was using to test which places do the copy-construction thing

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Oh, and the compiler not telling me what it's complaining about makes it
> hard to get past this to see what other areas might have problems with this.

Is the compiler clang?  clang seems to provide errors at every level of the template expansion stack, from initial source line with no expansion, all the way through every expansion to the final nightmarish 2000 character resulting type.  gcc only seems to provide the nightmares.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Compiler error output

> Is the compiler clang?

Yes, which is why I was rather surprised at the lack of information about who's really instantiating the template.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information