1299392 - stylo: Make owned/borrowed types safer

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Manish Goregaokar [:manishearth]]

Companion bug to https://github.com/servo/servo/pull/12991

Comment 1

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

Review commit: https://reviewboard.mozilla.org/r/75568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75568/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

https://reviewboard.mozilla.org/r/75568/#review73504

(Taking a look by Manish's request).

::: layout/style/ServoBindings.h:113
(Diff revision 1)
> +#undef DECL_OWNED_REF_TYPE_FOR
> +#undef DECL_Maybe_OWNED_REF_TYPE_FOR
> +#undef DECL_BORROWED_REF_TYPE_FOR
> +#undef DECL_Maybe_BORROWED_REF_TYPE_FOR
> +#undef DECL_BORROWED_MUT_REF_TYPE_FOR
> +#undef DECL_Maybe_BORROWED_MUT_REF_TYPE_FOR

Is it me, or this should be all uppercase? (otherwise you're `#undef`ing nothing).

::: layout/style/ServoBindings.cpp:519
(Diff revision 1)
>    *aClassList = rawElements;
>    return atomArray->Length();
>  }
>  
>  #define SERVO_IMPL_ELEMENT_ATTR_MATCHING_FUNCTIONS(prefix_, implementor_)      \
> -  nsIAtom* prefix_##AtomAttrValue(implementor_* aElement, nsIAtom* aName)      \
> +  nsIAtom* prefix_##AtomAttrValue(implementor_ aElement, nsIAtom* aName)      \

Please keep the alignment here :-)

Comment 3

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75568/diff/1-2/

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75568/diff/2-3/

Comment 5

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

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

https://reviewboard.mozilla.org/r/75568/#review73602

LGTM - I just wish we had a better way to integrate across the language boundary without defining all of these specialized typedefs... I don't think we do though.

::: layout/style/ServoBindings.h:66
(Diff revision 3)
>  }
>  }
>  
> -#define DECL_REF_TYPE_FOR(type_)          \
> -  typedef type_* type_##Borrowed;         \
> +using mozilla::dom::StyleChildrenIterator;
> +
> +#define DECL_BORROWED_REF_TYPE_FOR(type_) typedef type_* type_##Borrowed;

Can we have a comment here explaining how these types correspond with types on the servo side, and perhaps why they exist?

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

https://reviewboard.mozilla.org/r/75568/#review73736

This generally looks right, but there are enough things that should become non-nullable that I'd like to see the patch again with that.

::: layout/style/ServoBindingList.h:22
(Diff revision 3)
>   * SERVO_BINDING_FUNC(name_, return_, ...)
>   * before including this file.
>   */
>  
>  // Node data
> -SERVO_BINDING_FUNC(Servo_NodeData_Drop, void, ServoNodeData* data)
> +SERVO_BINDING_FUNC(Servo_NodeData_Drop, void, ServoNodeDataMaybeOwned data)

I think MaybeOwned/MaybeBorrowed are not-great names, because the "Maybe" would seem to apply to the ownership semantics, not nullability.

So let's replace the conventioned with OwnedOrNull and BorrowedOrNull.

::: layout/style/ServoBindingList.h:32
(Diff revision 3)
>  SERVO_BINDING_FUNC(Servo_StyleSheet_AddRef, void,
> -                   RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSheetMaybeBorrowed sheet)
>  SERVO_BINDING_FUNC(Servo_StyleSheet_Release, void,
> -                   RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSheetMaybeBorrowed sheet)

AddRef and Release shouldn't accept nullable types.

::: layout/style/ServoBindingList.h:37
(Diff revision 3)
>  SERVO_BINDING_FUNC(Servo_StyleSheet_AddRef, void,
> -                   RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSheetMaybeBorrowed sheet)
>  SERVO_BINDING_FUNC(Servo_StyleSheet_Release, void,
> -                   RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSheetMaybeBorrowed sheet)
>  SERVO_BINDING_FUNC(Servo_StyleSheet_HasRules, bool,
> -                   RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSheetMaybeBorrowed sheet)

This shouldn't be nullable.

::: layout/style/ServoBindingList.h:40
(Diff revision 3)
>  SERVO_BINDING_FUNC(Servo_StyleSet_AppendStyleSheet, void,
> -                   RawServoStyleSet* set, RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSetBorrowedMut set, RawServoStyleSheetMaybeBorrowed sheet)
>  SERVO_BINDING_FUNC(Servo_StyleSet_PrependStyleSheet, void,
> -                   RawServoStyleSet* set, RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSetBorrowedMut set, RawServoStyleSheetMaybeBorrowed sheet)
>  SERVO_BINDING_FUNC(Servo_StyleSet_RemoveStyleSheet, void,
> -                   RawServoStyleSet* set, RawServoStyleSheetBorrowed sheet)
> +                   RawServoStyleSetBorrowedMut set, RawServoStyleSheetMaybeBorrowed sheet)
>  SERVO_BINDING_FUNC(Servo_StyleSet_InsertStyleSheetBefore, void,
> -                   RawServoStyleSet* set, RawServoStyleSheetBorrowed sheet,
> -                   RawServoStyleSheetBorrowed reference)
> +                   RawServoStyleSetBorrowedMut set, RawServoStyleSheetMaybeBorrowed sheet,
> +                   RawServoStyleSheetMaybeBorrowed reference)

None of these should be nullable.

There are various other places in this file where this comment applies, so please just go through all the types and make sure that we only have nullability where it makes sense.

::: layout/style/ServoBindings.h:96
(Diff revision 3)
> +DECL_ARC_REF_TYPE_FOR(ServoDeclarationBlock)
> +
> +DECL_OWNED_REF_TYPE_FOR(RawServoStyleSet)
> +DECL_MAYBE_OWNED_REF_TYPE_FOR(ServoNodeData)
> +
> +// We don't use BorrowedMut because the nodes may alias

Moreover, the nodes are immutable from Servo modulo modifying the ServoNodeData.

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75568/diff/3-4/

Comment 8

[Manish Goregaokar [:manishearth]]

> Moreover, the nodes are immutable from Servo modulo modifying the ServoNodeData.

We also don't read from it (otherwise noalias would cause problems).

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75568/diff/4-5/

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

https://reviewboard.mozilla.org/r/75568/#review74042

::: layout/style/ServoBindingList.h:22
(Diff revision 5)
>   * SERVO_BINDING_FUNC(name_, return_, ...)
>   * before including this file.
>   */
>  
>  // Node data
> -SERVO_BINDING_FUNC(Servo_NodeData_Drop, void, ServoNodeData* data)
> +SERVO_BINDING_FUNC(Servo_NodeData_Drop, void, ServoNodeDataOwnedOrNull data)

This shouldn't be nullable.

::: layout/style/ServoBindingList.h:97
(Diff revision 5)
>  SERVO_BINDING_FUNC(Servo_Shutdown, void)
>  
>  // Restyle hints
>  SERVO_BINDING_FUNC(Servo_ComputeRestyleHint, nsRestyleHint,
>                     RawGeckoElement* element, ServoElementSnapshot* snapshot,
> -                   RawServoStyleSet* set)
> +                   RawServoStyleSetBorrowedMut set)

This doesn't actually need to be mut (even though the servo impl casts it as such). Fix it or file a followup?

::: layout/style/ServoBindings.h:78
(Diff revision 5)
> +// The "Arc" types are Servo-managed Arc<ServoType>s, which are passed
> +// over FFI as Strong<T> (which is nullable).

It would be nice to have a distinction between nullable and non-nullable Strong<T>. If this is non-trivial, maybe file a followup?

::: layout/style/ServoBindings.h:112
(Diff revision 5)
> +// We don't use BorrowedMut because the nodes may alias
> +// Servo itself doesn't directly read or mutate these;
> +// it only asks Gecko to do so.

We may actually use the bindings at some point to twiddle the node flags directly rather than getting / setting them over FFI. Would this cause hazards? Would be good to update the comment to explain what we'd need to do there.

::: layout/style/ServoBindings.h:179
(Diff revision 5)
>  // first child. This generally works, but misses anonymous children, which we
>  // want to traverse during styling. To support these cases, we create an
>  // optional heap-allocated iterator for nodes that need it. If the creation
>  // method returns null, Servo falls back to the aforementioned simpler (and
>  // faster) sibling traversal.
> -mozilla::dom::StyleChildrenIterator* Gecko_MaybeCreateStyleChildrenIterator(RawGeckoNode* node);
> +StyleChildrenIteratorBorrowedOrNull Gecko_MaybeCreateStyleChildrenIterator(RawGeckoNodeBorrowed node);

This needs to be OwnedOrNull, not BorrowedOrNull. Does getting this wrong create hazards?

::: layout/style/ServoBindings.h:180
(Diff revision 5)
>  // want to traverse during styling. To support these cases, we create an
>  // optional heap-allocated iterator for nodes that need it. If the creation
>  // method returns null, Servo falls back to the aforementioned simpler (and
>  // faster) sibling traversal.
> -mozilla::dom::StyleChildrenIterator* Gecko_MaybeCreateStyleChildrenIterator(RawGeckoNode* node);
> -void Gecko_DropStyleChildrenIterator(mozilla::dom::StyleChildrenIterator* it);
> +StyleChildrenIteratorBorrowedOrNull Gecko_MaybeCreateStyleChildrenIterator(RawGeckoNodeBorrowed node);
> +void Gecko_DropStyleChildrenIterator(StyleChildrenIterator* it);

This should be StyleChildrenIteratorOwned.

::: layout/style/ServoBindings.h:225
(Diff revision 5)
>  // Style attributes.
> -ServoDeclarationBlockBorrowed Gecko_GetServoDeclarationBlock(RawGeckoElement* element);
> +ServoDeclarationBlockBorrowedOrNull Gecko_GetServoDeclarationBlock(RawGeckoElementBorrowed element);
>  
>  // Node data.
> -ServoNodeData* Gecko_GetNodeData(RawGeckoNode* node);
> -void Gecko_SetNodeData(RawGeckoNode* node, ServoNodeData* data);
> +ServoNodeDataBorrowedOrNull Gecko_GetNodeData(RawGeckoNodeBorrowed node);
> +void Gecko_SetNodeData(RawGeckoNodeBorrowed node, ServoNodeDataOwnedOrNull data);

Do we ever pass null in this case?

::: layout/style/ServoBindings.h:284
(Diff revision 5)
>  // Also, we might want a ComputedValues to ComputedValues API for animations?
>  // Not if we do them in Gecko...
> -nsStyleContext* Gecko_GetStyleContext(RawGeckoNode* node,
> +nsStyleContext* Gecko_GetStyleContext(RawGeckoNodeBorrowed node,
>                                        nsIAtom* aPseudoTagOrNull);
>  nsChangeHint Gecko_CalcStyleDifference(nsStyleContext* oldstyle,
> -                                       ServoComputedValuesBorrowed newstyle);
> +                                       ServoComputedValuesBorrowedOrNull newstyle);

This shouldn't be nullable.

::: layout/style/ServoBindings.cpp:129
(Diff revision 5)
> -Gecko_GetDocumentElement(RawGeckoDocument* aDoc)
> +Gecko_GetDocumentElement(RawGeckoDocumentBorrowed aDoc)
>  {
>    return aDoc->GetDocumentElement();
>  }
>  
> -StyleChildrenIterator*
> +StyleChildrenIteratorBorrowedMut

This is incorrect, but also different from the declaration in the header, which is also incorrect.

How can we catch bugs like this?

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

https://reviewboard.mozilla.org/r/75568/#review74042

> This doesn't actually need to be mut (even though the servo impl casts it as such). Fix it or file a followup?

It is mutable, because we flush_stylesheets, which is a mutating operation, no?

Not sure what we'd do in the followup.

> It would be nice to have a distinction between nullable and non-nullable Strong<T>. If this is non-trivial, maybe file a followup?

We don't have non-nullable Strong<T> types being passed over FFI yet. I'm going to do a bunch of renaming here in a followup that is pure servo-side (Borrowed -> BorrowedOrNull, etc), I'll add that type when I do so.

> This is incorrect, but also different from the declaration in the header, which is also incorrect.
> 
> How can we catch bugs like this?

Newtype it with a conversion constructor and a cast operator? The only reason it doesn't complain is because it's a simple typedef.

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75568/diff/5-6/

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

https://reviewboard.mozilla.org/r/75568/#review74172

::: layout/style/ServoBindings.h:81
(Diff revision 6)
> +#define DECL_BORROWED_REF_TYPE_FOR(type_) typedef type_* type_##Borrowed;
> +#define DECL_NULLABLE_BORROWED_REF_TYPE_FOR(type_) typedef type_* type_##BorrowedOrNull;
> +#define DECL_BORROWED_MUT_REF_TYPE_FOR(type_) typedef type_* type_##BorrowedMut;
> +#define DECL_NULLABLE_BORROWED_MUT_REF_TYPE_FOR(type_) typedef type_* type_##BorrowedMutOrNull;

I guess I can live with this for now, but can you file a followup for the newtype thing? I think it's pretty important.

::: layout/style/ServoBindings.cpp:904
(Diff revision 6)
>  #undef SERVO_BINDING_FUNC
>  #endif
>  
>  #ifdef MOZ_STYLO
>  const nsStyleVariables*
> -Servo_GetStyleVariables(ServoComputedValuesBorrowed aComputedValues)
> +Servo_GetStyleVariables(ServoComputedValuesBorrowedOrNull aComputedValues)

This shouldn't be nullable.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Manish Goregaokar [:manishearth] from comment #11)
> Comment on attachment 8786635 [details]
> Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for
> nodes/documents/iterators;
> 
> https://reviewboard.mozilla.org/r/75568/#review74042
> 
> > This doesn't actually need to be mut (even though the servo impl casts it as such). Fix it or file a followup?
> 
> It is mutable, because we flush_stylesheets, which is a mutating operation,
> no?

We don't do this in Servo_ComputeRestyleHint AFAIK. Though I wonder if we should?

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75568/diff/6-7/

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8786635 [details]
Bug 1299392 - stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators;

https://reviewboard.mozilla.org/r/75568/#review74172

> I guess I can live with this for now, but can you file a followup for the newtype thing? I think it's pretty important.

bug 1300006

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/249ed3e46aa2
stylo: Add safety glue for borrowed and owned types, use for nodes/documents/iterators; r=bholley,mystor

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/249ed3e46aa2