1273838 - add support for -moz-binding being set from Servo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Part 1: Make construction of URLValue without initial nsIURIs thread-safe.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add stylo bindings glue for refcounting nsIPrincipals and nsIURIs.

I used typedefs to represent whether a given IMPL_NSISUPPORTS thing defined a thread-safe mRefCnt because I was getting warnings about unused variables when I tried using a const.

I didn't add macros to define Gecko_{AddRef,Release}FooMainThread since I didn't need them yet.  NS_ASSERT_OWNINGTHEAD didn't work in them either, since that wants to look at _mOwningThread, which is private.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 3: Pass sheet base/referrer/principal to Servo_StylesheetFromUTF8Bytes.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add FFI set/copy methods for -moz-binding.

Comment 5

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b82b4a96526e

The Servo-side stuff I'll use is at https://github.com/heycam/servo/commits/moz-binding-stylo-2 but needs some cleaning up before I post a PR.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8754151 [details] [diff] [review]
Part 1: Make construction of URLValue without initial nsIURIs thread-safe.

Review of attachment 8754151 [details] [diff] [review]:
-----------------------------------------------------------------

How perf-sensitive is URLValue? Given the stuff about not making the destructor virtual, I'm a bit worried about adding extra allocations in the constructor for the holders, but maybe it's unavoidable.

::: layout/style/nsCSSValue.cpp
@@ +2498,5 @@
> +css::URLValueData::URLValueData(
> +        nsIURI* aURI,
> +        nsStringBuffer* aString,
> +        nsMainThreadPtrHandle<nsIURI> aReferrer,
> +        nsMainThreadPtrHandle<nsIPrincipal> aOriginPrincipal)

Instead of passing handles by value, I think we should take an already_AddRefed<nsMainThreadPtrHolder<nsIPrincipal>>, and put a do_AddRef around the |new| instantiation at the callsite. That may require adding a constructor overload to nsMainThreadPtrHandle, but I think it's worthwhile (and we can do the same for Holder).

Also, while we're touching nsProxyRelease.h, what do you think about typedefing nsMainThreadPtr{Handle,Holder} as mozilla::Ptr{Handle,Holder}? That'd sure make the usage a lot less verbose.

Those changes might as well go in a separate patch.

@@ +2513,5 @@
> +        nsStringBuffer* aString,
> +        nsMainThreadPtrHandle<nsIURI> aBaseURI,
> +        nsMainThreadPtrHandle<nsIURI> aReferrer,
> +        nsMainThreadPtrHandle<nsIPrincipal> aOriginPrincipal)
> +  : mURI(aBaseURI)

How does this compile if mURI is an nsCOMPtr?

@@ +2537,5 @@
>              NS_SUCCEEDED(mURI->Equals(aOther.mURI, &eq)) &&
>              eq)) &&
>            (mOriginPrincipal == aOther.mOriginPrincipal ||
> +           (NS_SUCCEEDED(self.mOriginPrincipal.get()->Equals(
> +                           other.mOriginPrincipal.get(), &eq)) && eq));

Please use the bool-valued overload of ::Equals, here and below.

And while you're at it, please just make that overload accept const things and get rid of the casting at the callsites.

@@ +2609,5 @@
> +                   new nsMainThreadPtrHolder<nsIURI>(aReferrer)),
> +                 nsMainThreadPtrHandle<nsIPrincipal>(
> +                   new nsMainThreadPtrHolder<nsIPrincipal>(aOriginPrincipal)))
> +{
> +}

I'd NS_AssertMainThread() here and below.

::: layout/style/nsCSSValue.h
@@ +122,5 @@
>  private:
>    // If mURIResolved is false, mURI stores the base URI.
>    // If mURIResolved is true, mURI stores the URI we resolve to; this may be
>    // null if the URI is invalid.
>    mutable nsCOMPtr<nsIURI> mURI;

Doesn't this need to be a PtrHandle? If not, why are we passing the base URI as a handle?

@@ +147,2 @@
>    URLValue(nsIURI* aURI, nsStringBuffer* aString, nsIURI* aReferrer,
> +           nsIPrincipal* aOriginPrincipal);

Add a comment indicating that the second and third constructors are intended to be called on the main thread, and the first can be called on any thread? Might be good to put the any-thread one last, since most consumers won't use it.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8754154 [details] [diff] [review]
Part 2: Add stylo bindings glue for refcounting nsIPrincipals and nsIURIs.

Review of attachment 8754154 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those fixes.

::: layout/style/ServoBindings.h
@@ +24,5 @@
>  typedef nsINode RawGeckoNode;
> +class nsIPrincipal;
> +typedef nsIPrincipal RawGeckoPrincipal;
> +class nsIURI;
> +typedef nsIURI RawGeckoURI;

I think it's probably fine (and less confusing) to just use nsIPrincipal and nsIURI directly.

The RawGeckoElement stuff was necessary before bindgen supported namespaces, though it might still be useful to keep the convention for Node/Element/Document.

@@ +64,5 @@
> +
> +#define NS_DECL_HOLDER_FFI_REFCOUNTING(class_)                                \
> +  NS_DECL_HOLDER_FFI_REFCOUNTING_WITH_NAME(class_, name_)
> +#define NS_IMPL_HOLDER_FFI_REFCOUNTING(class_)                                \
> +  NS_IMPL_HOLDER_FFI_REFCOUNTING_WITH_NAME(class_, name_)

Will we ever actually use these? I think it might be nicer to just get rid of these and drop the _WITH_NAME suffix on the other macros. If we ever happen to have a caller that wants this, they can just pass the same thing for both arguments.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8754156 [details] [diff] [review]
Part 3: Pass sheet base/referrer/principal to Servo_StylesheetFromUTF8Bytes.

Review of attachment 8754156 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoBindings.h
@@ +114,5 @@
>  RawServoStyleSheet* Servo_StylesheetFromUTF8Bytes(const uint8_t* bytes, uint32_t length,
> +                                                  mozilla::css::SheetParsingMode parsing_mode,
> +                                                  ThreadSafeURIHolder* base,
> +                                                  ThreadSafeURIHolder* referrer,
> +                                                  ThreadSafePrincipalHolder* principal);

It would be better to pass already_AddRefed versions here, since that would allow the servo code to hold a strong reference without an extra addref. I'd be ok with this as a followup though if it's too much to do here.

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #6)
> How perf-sensitive is URLValue? Given the stuff about not making the
> destructor virtual, I'm a bit worried about adding extra allocations in the
> constructor for the holders, but maybe it's unavoidable.

I would be surprised if it's perf-sensitive, really.

> @@ +2513,5 @@
> > +        nsStringBuffer* aString,
> > +        nsMainThreadPtrHandle<nsIURI> aBaseURI,
> > +        nsMainThreadPtrHandle<nsIURI> aReferrer,
> > +        nsMainThreadPtrHandle<nsIPrincipal> aOriginPrincipal)
> > +  : mURI(aBaseURI)
> 
> How does this compile if mURI is an nsCOMPtr?

nsMainThreadPtrHandle has an |operator T*()| method.

> @@ +2537,5 @@
> >              NS_SUCCEEDED(mURI->Equals(aOther.mURI, &eq)) &&
> >              eq)) &&
> >            (mOriginPrincipal == aOther.mOriginPrincipal ||
> > +           (NS_SUCCEEDED(self.mOriginPrincipal.get()->Equals(
> > +                           other.mOriginPrincipal.get(), &eq)) && eq));
> 
> Please use the bool-valued overload of ::Equals, here and below.

Which overload?  (Can you point me to it?)

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #6)
> ::: layout/style/nsCSSValue.h
> @@ +122,5 @@
> >  private:
> >    // If mURIResolved is false, mURI stores the base URI.
> >    // If mURIResolved is true, mURI stores the URI we resolve to; this may be
> >    // null if the URI is invalid.
> >    mutable nsCOMPtr<nsIURI> mURI;
> 
> Doesn't this need to be a PtrHandle? If not, why are we passing the base URI
> as a handle?

Hmm, yes it does.  (Or the base URI would need to be stored separately.)

Comment 11

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #9)
> > Please use the bool-valued overload of ::Equals, here and below.
> 
> Which overload?  (Can you point me to it?)

http://searchfox.org/mozilla-central/rev/e07e80ac43fb06913305890fed3aa9273e7533f0/caps/nsIPrincipal.idl#40

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8754157 [details] [diff] [review]
Part 4: Add FFI set/copy methods for -moz-binding.

Review of attachment 8754157 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those fixes.

::: layout/style/ServoBindings.cpp
@@ +235,5 @@
> +  nsString url;
> +  nsDependentCSubstring urlString(reinterpret_cast<const char*>(aURLString),
> +                                  aURLStringLength);
> +  AppendUTF8toUTF16(urlString, url);
> +  RefPtr<nsStringBuffer> urlBuffer = nsCSSValue::BufferFromString(url);

Can't we just call nsStringBuffer::FromString and assert that the result is non-null?

@@ +241,5 @@
> +  aDisplay->mBinding =
> +    new css::URLValue(urlBuffer,
> +                      nsMainThreadPtrHandle<nsIURI>(aBaseURI),
> +                      nsMainThreadPtrHandle<nsIURI>(aReferrer),
> +                      nsMainThreadPtrHandle<nsIPrincipal>(aPrincipal));

I'd rather assign the args into refptrs an and then pass .forget() for these here.

::: layout/style/ServoBindings.h
@@ +108,5 @@
>  NS_DECL_HOLDER_FFI_REFCOUNTING_WITH_NAME(RawGeckoURI, URI)
>  
> +// Display style.
> +struct nsStyleDisplay;
> +void Gecko_SetBinding(nsStyleDisplay* style_struct,

Can we call this SetXBLBinding or SetMozBinding? SetBinding sounds more general than this actually is. Same with Copy below.

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #6)
> And while you're at it, please just make that overload accept const things
> and get rid of the casting at the callsites.

The overload which takes the outparam, which this overload defers to, is also not const and takes a non-const argument.  Is is safe to cast away const inside there?

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 0.8: Add nsMainThreadPtrHandle constructor that takes an already_AddRefed nsMainThreadPtrHolder.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 0.9: Add Ptr{Holder,Handle} typedefs for nsMainThreadPtr{Holder,Handle}.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 1: Make URLValue construction thread-safe.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #13)
> The overload which takes the outparam, which this overload defers to, is
> also not const and takes a non-const argument.  Is is safe to cast away
> const inside there?

Just as safe as it is to cast it away at the callsite like your original patch does. But yeah, it should be fine.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8754336 [details] [diff] [review]
Part 0.8: Add nsMainThreadPtrHandle constructor that takes an already_AddRefed nsMainThreadPtrHolder.

Review of attachment 8754336 [details] [diff] [review]:
-----------------------------------------------------------------

Also might be worth adding the same for nsMainThreadPtrHolder?

::: xpcom/glue/nsProxyRelease.h
@@ +229,5 @@
>      : mPtr(aHolder)
>    {
>    }
> +  explicit nsMainThreadPtrHandle(already_AddRefed<nsMainThreadPtrHolder<T>>
> +                                   aHolder)

Nit: This alignment looks weird.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8754338 [details] [diff] [review]
Part 1: Make URLValue construction thread-safe.

This appears to be the same patch as the original part 1.

Comment 20

[Cameron McCormack (:heycam)]

Attachment: Part 1: Make URLValue construction thread-safe.

Comment 21

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #17)
> (In reply to Cameron McCormack (:heycam) from comment #13)
> > The overload which takes the outparam, which this overload defers to, is
> > also not const and takes a non-const argument.  Is is safe to cast away
> > const inside there?
> 
> Just as safe as it is to cast it away at the callsite like your original
> patch does. But yeah, it should be fine.

OK.  I'll do that in a followup.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8754613 [details] [diff] [review]
Part 1: Make URLValue construction thread-safe.

Review of attachment 8754613 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSValue.cpp
@@ +2568,5 @@
>      nsCOMPtr<nsIURI> newURI;
>      NS_NewURI(getter_AddRefs(newURI),
>                NS_ConvertUTF16toUTF8(nsCSSValue::GetBufferValue(mString)),
>                nullptr, mURI);
> +    mURI = new PtrHolder<nsIURI>(newURI);

This should be newURI.forget(), right?

Comment 23

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (busy) from comment #8)
> It would be better to pass already_AddRefed versions here, since that would
> allow the servo code to hold a strong reference without an extra addref. I'd
> be ok with this as a followup though if it's too much to do here.

This would have to be a pointer to an already_AddRefed object, right?  I can't pass an already_AddRefed object itself to an extern "C" function I don't think.  So I would have to store the already_AddRefed objects in a local variable before passing them (since you can't take the address of a temporary).

Alternatively I just the arguments as ThreadSafeURIHolder* and ThreadSafePrincipalHolder* but just promise that they have already been AddRefed.  That feels a bit unsafe, so maybe I will just leave them as is, unless I've missed a better option.

Comment 24

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e18ff2fb155c

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a358ce1ba2
https://hg.mozilla.org/integration/mozilla-inbound/rev/01409116ceda
https://hg.mozilla.org/integration/mozilla-inbound/rev/92a94c8a2155
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8833469c11
https://hg.mozilla.org/integration/mozilla-inbound/rev/3039198867c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c73a31a8516

Comment 26

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #23)
> (In reply to Bobby Holley (busy) from comment #8)
> This would have to be a pointer to an already_AddRefed object, right?  I
> can't pass an already_AddRefed object itself to an extern "C" function I
> don't think.  So I would have to store the already_AddRefed objects in a
> local variable before passing them (since you can't take the address of a
> temporary).

Oh yeah, that's annoying.

It seems like we should be able to design a typesafe way to express already_AddRefed over FFI. Possibly with typedef-ed pointers and/or help from bindgen.

Emilio, is this something you'd be interested in looking into? I think it would make our implementation a lot safer.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (busy) from comment #26)
> (In reply to Cameron McCormack (:heycam) from comment #23)
> > (In reply to Bobby Holley (busy) from comment #8)
> > This would have to be a pointer to an already_AddRefed object, right?  I
> > can't pass an already_AddRefed object itself to an extern "C" function I
> > don't think.  So I would have to store the already_AddRefed objects in a
> > local variable before passing them (since you can't take the address of a
> > temporary).
> 
> Oh yeah, that's annoying.
> 
> It seems like we should be able to design a typesafe way to express
> already_AddRefed over FFI. Possibly with typedef-ed pointers and/or help
> from bindgen.
> 
> Emilio, is this something you'd be interested in looking into? I think it
> would make our implementation a lot safer.

Sounds good, shouldn't be *that* hard. Just did a quick try and we generate correct bindings to already_AddRefed.

Once we have that, We can manually implement Deref in the rust side to get a reference with the correct lifetime, and some convenience methods.

I haven't too much time these weeks, but I think I can have it pretty soon, does that sound sensible? Or were you thinking about something else?

Comment 28

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> (In reply to Bobby Holley (busy) from comment #26)
> > (In reply to Cameron McCormack (:heycam) from comment #23)
> > > (In reply to Bobby Holley (busy) from comment #8)
> > > This would have to be a pointer to an already_AddRefed object, right?  I
> > > can't pass an already_AddRefed object itself to an extern "C" function I
> > > don't think.  So I would have to store the already_AddRefed objects in a
> > > local variable before passing them (since you can't take the address of a
> > > temporary).
> > 
> > Oh yeah, that's annoying.
> > 
> > It seems like we should be able to design a typesafe way to express
> > already_AddRefed over FFI. Possibly with typedef-ed pointers and/or help
> > from bindgen.
> > 
> > Emilio, is this something you'd be interested in looking into? I think it
> > would make our implementation a lot safer.
> 
> Sounds good, shouldn't be *that* hard. Just did a quick try and we generate
> correct bindings to already_AddRefed.

Right, but the issue heycam discovered is that we can't pass structs by value to extern "C" functions.

So I think we want either a template-based or macro-based mechanism to generate an incompatible pointer type U such that we can somehow convert already_AddRefed<T> to U*, but that T* doesn't convert to U* even though they're secretly the same thing.

> Once we have that, We can manually implement Deref in the rust side to get a
> reference with the correct lifetime, and some convenience methods.

 
> I haven't too much time these weeks, but I think I can have it pretty soon,
> does that sound sensible? Or were you thinking about something else?

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3a358ce1ba2
https://hg.mozilla.org/mozilla-central/rev/01409116ceda
https://hg.mozilla.org/mozilla-central/rev/92a94c8a2155
https://hg.mozilla.org/mozilla-central/rev/1f8833469c11
https://hg.mozilla.org/mozilla-central/rev/3039198867c1
https://hg.mozilla.org/mozilla-central/rev/6c73a31a8516