Closed Bug 1309109 Opened 8 years ago Closed 8 years ago

stylo: Move {Gecko,Servo}DeclarationBlock into the Gecko side

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(7 files, 1 obsolete file)

Bug 1309109 part 1 - Change underlying type of StyleBackendType to uint8_t.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1309109 part 2 - Move immutable and container from css::Declaration to a new base class.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1309109 part 3 - Rename ServoDeclarationBlock to RawServoDeclarationBlock.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1309109 part 4 - Add ServoDeclarationBlock class.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1309109 part 5 - Store ServoDeclarationBlock rather than RawServoDeclarationBlock in nsAttrValue.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1309109 part 7 - Remove the extra level of GeckoDeclarationBlock.
58 bytes, text/x-review-board-request
manishearth
: review+
nika
: review+
Details
Bug 1309109 part 6 - Declare nullable strong borrowed ref type for RawServoDeclarationBlock.
58 bytes, text/x-review-board-request
nika
: review+
Details 
After servo/servo#13134 [1], GeckoDeclarationBlock (ServoDeclarationBlock in the Gecko side) holds the native Servo PropertyDeclarationBlock via an Arc. The other two fields, cache and immutable, are only used from the Gecko side.

I think it would be easier to move the whole struct from Servo side to Gecko side, so that we don't need Atomic workarounds for those fields, and we don't need to go across FFI boundary for reading/writing them.


[1] https://github.com/servo/servo/pull/13134
Assignee: nobody → xidorn+moz
Blocks: 1307357
Depends on: 1309202
Manish: The last patch isn't quite ready. I see weird crash with that patch, and I've got stuck on it for about a whole day without much progress, so I decided to publish it and see if you have any suggestion.

It seems to me the reason is that the content inside the RwLock<PropertyDeclarationBlock> is somehow broken, but I have no idea how that happens...
Flags: needinfo?(manishearth)
Comment on attachment 8800475 [details]
Bug 1309109 part 2 - Move immutable and container from css::Declaration to a new base class.

https://reviewboard.mozilla.org/r/85370/#review83962

::: layout/style/Declaration.h:87
(Diff revision 1)
>  // call) when it is matched (put in the rule tree); after that, it must
>  // be copied before it can be modified, which is taken care of by
>  // |EnsureMutable|.
>  
> -class Declaration final : public nsIStyleRule {
> +class Declaration final : public DeclarationBlock
> +                        , public nsIStyleRule

Totally drive-by and missing-sleep comment. I don't know if it's important anymore, but I thought the nsIThings always need to go first in the inheritance chain?
Comment on attachment 8800475 [details]
Bug 1309109 part 2 - Move immutable and container from css::Declaration to a new base class.

https://reviewboard.mozilla.org/r/85370/#review83962

> Totally drive-by and missing-sleep comment. I don't know if it's important anymore, but I thought the nsIThings always need to go first in the inheritance chain?

I have no idea. Is there any document says that we need to do that, and explain why? Probably there are some magic on nsISupports?
The last patch doesn't seem broken to me, hmm. Could you provide more information about the crash?
Flags: needinfo?(manishearth)
Comment on attachment 8800475 [details]
Bug 1309109 part 2 - Move immutable and container from css::Declaration to a new base class.

https://reviewboard.mozilla.org/r/85370/#review83962

> I have no idea. Is there any document says that we need to do that, and explain why? Probably there are some magic on nsISupports?

I'm pretty sure I read that somewhere, but I can't find it right now. I guess it's so the vtable layout is consistent among nsISupports yeah, though I'm not sure it matters in this case, since Declaration doesn't have any virtual method.
Comment on attachment 8800479 [details]
Bug 1309109 part 7 - Remove the extra level of GeckoDeclarationBlock.

https://reviewboard.mozilla.org/r/85378/#review84102

::: servo/components/style/gecko/wrapper.rs:461
(Diff revision 1)
>          unsafe { GeckoNode(&*(self.0 as *const _ as *const RawGeckoNode)) }
>      }
>  
>      fn style_attribute(&self) -> Option<&Arc<RwLock<PropertyDeclarationBlock>>> {
>          let declarations = unsafe { Gecko_GetServoDeclarationBlock(self.0) };
> -        if declarations.is_none() {
> +        RwLock::<PropertyDeclarationBlock>::arc_from_borrowed(&declarations)

Regarding this patch, and being it a total shot in the dark, the style attribute declaration block is no longer stored in the applicable declarations cache since recent changes by Simon landed, so maybe this lifetime prolongation was unsound but was just covered by that?
Comment on attachment 8800479 [details]
Bug 1309109 part 7 - Remove the extra level of GeckoDeclarationBlock.

https://reviewboard.mozilla.org/r/85378/#review84102

> Regarding this patch, and being it a total shot in the dark, the style attribute declaration block is no longer stored in the applicable declarations cache since recent changes by Simon landed, so maybe this lifetime prolongation was unsound but was just covered by that?

Yeah, this is the issue causing the crash I was seeing. Manish pointed that out to me earlier today, and he gave me a solution for this. I'm working on new patches based on his solution.
Oh, sorry, we didn't leave a comment here: I diagnosed this this morning and traced it back to the same function, we discussed this in IRC and I came up with a solution.

The problem was that arc_from_borrowed gives a reference to the local stack variable `declarations` disguised as an arc, which subsequently has its lifetime prolonged. This was fine in the past due to the double-indirection; the reference was to something heap-allocated and was safe to use that way.


Rough fix is at https://gist.github.com/Manishearth/c63fb0d6b8db99d92515fb79e28830e3 . It conceptually returns a &Arc<T> from Gecko (a double pointer) and converts it on the servo side.
Attachment #8800477 - Flags: review?(cam)
Attachment #8800479 - Flags: review?(manishearth)
Depends on: 1309868
Comment on attachment 8800479 [details]
Bug 1309109 part 7 - Remove the extra level of GeckoDeclarationBlock.

https://reviewboard.mozilla.org/r/85378/#review84142

I wrote the previous patch and the line using into_arc_opt_borrow and the definition of Gecko_GetServoDeclarationBlock, so this r+ only applies to the other parts. @mystor, we need a safety review on these bits, thanks :)
Attachment #8800479 - Flags: review?(manishearth) → review+
Comment on attachment 8800674 [details]
Bug 1309109 part 4 - Add get_ref method for RefPtr to get reference of raw pointer.

https://reviewboard.mozilla.org/r/85518/#review84168

Why return use this method when you can just call `.get()` and get the same effect?  AFAICT, that's what you really want to do in part 5 and its uses in subsequent parts anyway.
Attachment #8800674 - Flags: review?(nfroyd)
Comment on attachment 8800674 [details]
Bug 1309109 part 4 - Add get_ref method for RefPtr to get reference of raw pointer.

https://reviewboard.mozilla.org/r/85518/#review84168

Because .get() doesn't return the reference to the pointer. What I actually need is a T** which points to a non-temporary location which stores a T*.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> Because .get() doesn't return the reference to the pointer. What I actually
> need is a T** which points to a non-temporary location which stores a T*.

Why do you need this?  I can't see any place that actually takes advantage of this fact.
Servo side wants a &Arc<T> (Arc is basically RefPtr) as return value from TElement::style_attribute, however, the T is hold in Gecko side, so we don't really have a non-temporary object in Servo side which stores the pointer to T, so we want to fake one via casting a T** to &Arc<T>. (The casting is safe here because T is managed only in Servo side.)

You can see part 8 for that.
Also see comment 12 and comment 14 (and probably the previous version of part 8).
> Servo side wants a &Arc<T> (Arc is basically RefPtr) as return value from TElement::style_attribute, however, the T is hold in Gecko side, so we don't really have a non-temporary object in Servo side which stores the pointer to T, so we want to fake one via casting a T** to &Arc<T>. (The casting is safe here because T is managed only in Servo side.)

We don't need get_ref for that though. We can just return `&mRaw` in SDB::Raw(), and later cast it to a double pointer.
I add a method to RefPtr rather than casting RefPtr<T> directly because I don't want it to silently rely on impl details of RefPtr.
Comment on attachment 8800479 [details]
Bug 1309109 part 7 - Remove the extra level of GeckoDeclarationBlock.

https://reviewboard.mozilla.org/r/85378/#review84174

I think that this is safe, so r+ from me. Not the biggest fan of the reinterpret_cast, but I couldn't think of a good way to get rid of it, so *shrug*. Generally I would like some more comments explaining what is going on here, especially related to the name of the type, because it can be really confusing (I spend a good while just trying to figure out what that type was).

::: layout/style/ServoBindings.cpp:327
(Diff revision 2)
> -  return decl->AsServo()->Raw();
> +  return reinterpret_cast<const RawServoDeclarationBlockStrong*>
> +    (&decl->AsServo()->Raw());

I assume that the memory you are taking a reference to here is being kept alive by aElement? 

I think this should be OK because lifetime elision will tie the input and output lifetimes together on the rust side.

Perhaps mention that Raw() returns an lvalue reference to the owning reference?
Attachment #8800479 - Flags: review?(michael) → review+
Comment on attachment 8800675 [details]
Bug 1309109 part 6 - Declare nullable strong borrowed ref type for RawServoDeclarationBlock.

https://reviewboard.mozilla.org/r/85520/#review84176

::: layout/style/ServoBindings.h:105
(Diff revision 1)
>    DECL_NULLABLE_BORROWED_MUT_REF_TYPE_FOR(type_)
>  
>  DECL_ARC_REF_TYPE_FOR(ServoComputedValues)
>  DECL_ARC_REF_TYPE_FOR(RawServoStyleSheet)
>  DECL_ARC_REF_TYPE_FOR(RawServoDeclarationBlock)
> +DECL_NULLABLE_BORROWED_REF_TYPE_FOR(RawServoDeclarationBlockStrong)

I am not a fan of the names of these types, but I understand that they are autogenerated so will be a pain to change. 

Is there any chance you could add some comments explaining what this type means? I eventually figured out that it was an `Option<&Arc<RawServoDeclarationBlock>>` but it took me a bit.

::: servo/components/style/gecko_bindings/sugar/ownership.rs:165
(Diff revision 1)
> +    /// Given a reference to a strong FFI reference,
> +    /// converts it into a reference to a servo-side Arc
> +    /// Returns None on null.
> +    ///
> +    /// Strong<GeckoType> -> Arc<ServoType>
> +    pub fn into_arc_opt_borrow<U>(&self) -> Option<&Arc<U>> where U: HasArcFFI<FFIType = T> {
> +        if self.is_null() {
> +            None
> +        } else {
> +            unsafe { Some(transmute(self)) }
> +        }
> +    }

LGTM, Really not a big fan of the name, we aren't `into`-ing anything, we're taking a reference. Perhaps `as_arc_opt`?
Attachment #8800675 - Flags: review?(michael) → review+
Comment on attachment 8800675 [details]
Bug 1309109 part 6 - Declare nullable strong borrowed ref type for RawServoDeclarationBlock.

https://reviewboard.mozilla.org/r/85520/#review84176

> I am not a fan of the names of these types, but I understand that they are autogenerated so will be a pain to change. 
> 
> Is there any chance you could add some comments explaining what this type means? I eventually figured out that it was an `Option<&Arc<RawServoDeclarationBlock>>` but it took me a bit.

+1, we need more comments explaining this situation. A comment on top of this macro invocation wfm.

(I didn't really pay attention to comments when writing this patch, mostly just wanted to get it to _work_. It would be nice to have comments on this and the implementation of Gecko_GetSDB as well.)
Given the explanation in comment 27 and comment 30, are you fine with part 4 now?
Flags: needinfo?(nfroyd)
Comment on attachment 8800477 [details]
Bug 1309109 part 4 - Add ServoDeclarationBlock class.

https://reviewboard.mozilla.org/r/85374/#review84404
Comment on attachment 8800478 [details]
Bug 1309109 part 5 - Store ServoDeclarationBlock rather than RawServoDeclarationBlock in nsAttrValue.

The DOM changes here look pretty mechanical. I think it's more useful to have the same person review all the parts here.
Attachment #8800478 - Flags: review?(bobbyholley) → review?(cam)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #34)
> Given the explanation in comment 27 and comment 30, are you fine with part 4
> now?

Where is the definition of RawServoDeclarationBlockStrongBorrowedOrNull?  I'm assuming it's autogenerated, but it would be helpful to know what it is, as somebody who's not familiar with the Stylo bindings story.

I think it would be better to have Stylo bits do any reinterpret_cast'ing gymnastics themselves, rather than adding a method to RefPtr that's only going to be used by Stylo.  The internals of RefPtr changing seem unlikely, and even if they did, that'd be a fundamental changes to Gecko that we'd have to carefully think about anyway.

A counter-argument is that other Rust-to-Gecko bindings might require a similar arrangement of returning strong refcounted pointers into Rust, and then having the method on RefPtr would make more sense--assuming people actually used it.
Flags: needinfo?(nfroyd)
> Where is the definition of RawServoDeclarationBlockStrongBorrowedOrNull?  I'm assuming it's autogenerated, but it would be helpful to know what it is, as somebody who's not familiar with the Stylo bindings story.

https://reviewboard.mozilla.org/r/85520/diff/1#index_header , bindings.rs

It's `Option<&Strong<RawServoDeclarationBlock>>`, where RawSDB is an opaque type, and `Strong<T>` is a pointer type. The `Option<&T>` is represented as a nullable pointer.

`Strong<T>` can be treated as `Arc<U>`, so this is actually `Option<&Arc<ServoDeclarationBlock>>`.

>  The internals of RefPtr changing seem unlikely, and even if they did, that'd be a fundamental changes to Gecko that we'd have to carefully think about anyway.


I personally am okay with this, I think some debug asserts can be used to ensure this doesn't change. We make similar assumptions elsewhere in stylo iirc.
(In reply to Manish Goregaokar [:manishearth] from comment #38)
> > Where is the definition of RawServoDeclarationBlockStrongBorrowedOrNull?  I'm assuming it's autogenerated, but it would be helpful to know what it is, as somebody who's not familiar with the Stylo bindings story.
> 
> https://reviewboard.mozilla.org/r/85520/diff/1#index_header , bindings.rs
> 
> It's `Option<&Strong<RawServoDeclarationBlock>>`, where RawSDB is an opaque
> type, and `Strong<T>` is a pointer type. The `Option<&T>` is represented as
> a nullable pointer.

Ah, I was asking for the Gecko side of things, but this pointed me in the right direction.  Thank you!
OK, if you are all happy with just reinterpret_cast'ing that, I can do that as well, although I still think explicitly adding a stable interface is the best.
Attachment #8800674 - Attachment is obsolete: true
Comment on attachment 8800474 [details]
Bug 1309109 part 1 - Change underlying type of StyleBackendType to uint8_t.

https://reviewboard.mozilla.org/r/85368/#review85346
Attachment #8800474 - Flags: review?(cam) → review+
Comment on attachment 8800475 [details]
Bug 1309109 part 2 - Move immutable and container from css::Declaration to a new base class.

https://reviewboard.mozilla.org/r/85370/#review85350
Attachment #8800475 - Flags: review?(cam) → review+
Comment on attachment 8800476 [details]
Bug 1309109 part 3 - Rename ServoDeclarationBlock to RawServoDeclarationBlock.

https://reviewboard.mozilla.org/r/85372/#review85352
Attachment #8800476 - Flags: review?(cam) → review+
Comment on attachment 8800477 [details]
Bug 1309109 part 4 - Add ServoDeclarationBlock class.

https://reviewboard.mozilla.org/r/85374/#review85354

::: layout/style/DeclarationBlock.h:16
(Diff revision 3)
>  
>  #ifndef mozilla_DeclarationBlock_h
>  #define mozilla_DeclarationBlock_h
>  
>  #include "mozilla/StyleBackendType.h"
> +#include "mozilla/ServoUtils.h"

Nit: one line up to keep this sorted.

::: layout/style/ServoDeclarationBlock.h:22
(Diff revision 3)
> +  RawServoDeclarationBlock* const& Raw() const {
> +    return *reinterpret_cast<RawServoDeclarationBlock* const*>(&mRaw);
> +  }

Why do we need to do this rather than just:

  const RawServoDeclarationBlock* Raw() const {
    return mRaw;
  }

?
Comment on attachment 8800477 [details]
Bug 1309109 part 4 - Add ServoDeclarationBlock class.

https://reviewboard.mozilla.org/r/85374/#review85354

> Why do we need to do this rather than just:
> 
>   const RawServoDeclarationBlock* Raw() const {
>     return mRaw;
>   }
> 
> ?

Firstly, we don't want a const pointer to "RawServoDeclarationBlock". What we really want is a pointer to "RawServoDeclarationBlock*", because Servo side code wants a non-temporary reference of RawServoDeclarationBlock. See comment 14 and comment 27.
Comment on attachment 8800477 [details]
Bug 1309109 part 4 - Add ServoDeclarationBlock class.

https://reviewboard.mozilla.org/r/85374/#review85354

> Firstly, we don't want a const pointer to "RawServoDeclarationBlock". What we really want is a pointer to "RawServoDeclarationBlock*", because Servo side code wants a non-temporary reference of RawServoDeclarationBlock. See comment 14 and comment 27.

That said, the reference itself should be non-temporary, so we need an address of an in-heap `RawServoDeclarationBlock*`, which is `RawServoDeclarationBlock**`. But I think we don't want to easily change the raw pointer stored inside RefPtr, so I make it `RawServoDeclarationBlock* const*`. (Actually you need an additional `const_cast` to remove the `const` here if you want.)
Comment on attachment 8800479 [details]
Bug 1309109 part 7 - Remove the extra level of GeckoDeclarationBlock.

https://reviewboard.mozilla.org/r/85378/#review84174

> I assume that the memory you are taking a reference to here is being kept alive by aElement? 
> 
> I think this should be OK because lifetime elision will tie the input and output lifetimes together on the rust side.
> 
> Perhaps mention that Raw() returns an lvalue reference to the owning reference?

Combining this comment with that heycam raised for part 4, I guess I really should rename Raw() and make it return a `RawServoDeclarationBlock* const*` instead of a reference to that. I don't have a good name for it in mind at the moment, though.
Comment on attachment 8800478 [details]
Bug 1309109 part 5 - Store ServoDeclarationBlock rather than RawServoDeclarationBlock in nsAttrValue.

https://reviewboard.mozilla.org/r/85376/#review85368

::: layout/style/ServoBindings.cpp:324
(Diff revision 3)
> +  if (!decl || decl->IsGecko()) {
> +    return nullptr;
> +  }

I think decl->IsGecko() shouldn't happen.  Can you assert it?
Attachment #8800478 - Flags: review?(cam) → review+
Comment on attachment 8800477 [details]
Bug 1309109 part 4 - Add ServoDeclarationBlock class.

https://reviewboard.mozilla.org/r/85374/#review85388

r=me with appropriate changes to Raw().
Attachment #8800477 - Flags: review?(cam) → review+
Comment on attachment 8800478 [details]
Bug 1309109 part 5 - Store ServoDeclarationBlock rather than RawServoDeclarationBlock in nsAttrValue.

https://reviewboard.mozilla.org/r/85376/#review85368

> I think decl->IsGecko() shouldn't happen.  Can you assert it?

I don't know why but it does happen... I'll have a look.
Comment on attachment 8800478 [details]
Bug 1309109 part 5 - Store ServoDeclarationBlock rather than RawServoDeclarationBlock in nsAttrValue.

https://reviewboard.mozilla.org/r/85376/#review85368

> I don't know why but it does happen... I'll have a look.

This can happen at least when the script tries to set style attribute, because the DOM object for that is not aware of stylo yet. So we cannot assert it at present, but we may want to change it later.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc64dced7300
part 1 - Change underlying type of StyleBackendType to uint8_t. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/32868c79880c
part 2 - Move immutable and container from css::Declaration to a new base class. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/9868b98cf99e
part 3 - Rename ServoDeclarationBlock to RawServoDeclarationBlock. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7414c28453
part 4 - Add ServoDeclarationBlock class. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/426e84d58021
part 5 - Store ServoDeclarationBlock rather than RawServoDeclarationBlock in nsAttrValue. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b167cfcb4f8f
part 6 - Declare nullable strong borrowed ref type for RawServoDeclarationBlock. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/346efffa3a80
part 7 - Remove the extra level of GeckoDeclarationBlock. r=manishearth,mystor
The Servo side change of this bug (and bug 1309868) is in PR: https://github.com/servo/servo/pull/13809
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: