Closed Bug 1273838 Opened 5 years ago Closed 5 years ago

add support for -moz-binding being set from Servo

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(6 files, 2 obsolete files)

No description provided.
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.
Attachment #8754154 - Flags: review?(bobbyholley)
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.
Attachment #8754151 - Flags: review?(bobbyholley) → review-
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.
Attachment #8754154 - Flags: review?(bobbyholley) → review+
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.
Attachment #8754156 - Flags: review?(bobbyholley) → review+
(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?)
(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.)
(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 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.
Attachment #8754157 - Flags: review?(bobbyholley) → review+
(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?
Attachment #8754151 - Attachment is obsolete: true
Attachment #8754338 - Flags: review?(bobbyholley)
Attachment #8754337 - Attachment description: 0.9: Add Ptr{Holder,Handle} typedefs for nsMainThreadPtr{Holder,Handle}. → Part 0.9: Add Ptr{Holder,Handle} typedefs for nsMainThreadPtr{Holder,Handle}.
(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 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.
Attachment #8754336 - Flags: review?(bobbyholley) → review+
Attachment #8754337 - Flags: review?(bobbyholley) → review+
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.
Attachment #8754338 - Flags: review?(bobbyholley) → review-
Attachment #8754338 - Attachment is obsolete: true
Attachment #8754613 - Flags: review?(bobbyholley)
(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 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?
Attachment #8754613 - Flags: review?(bobbyholley) → review+
(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.
(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.
Flags: needinfo?(ecoal95)
(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?
Flags: needinfo?(ecoal95) → needinfo?(bobbyholley)
(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?
Flags: needinfo?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.