Closed Bug 1329093 Opened 7 years ago Closed 7 years ago

stylo: Need a Stylo version of SVG's MappedAttrParser and attendant functionality

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: manishearth)

References

Details

Attachments

(4 files, 2 obsolete files)

Bug 1329093 - Part 1: stylo: Handle SVG presentation attributes;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1329093 - Part 3: Overload FromContent() to work with const;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later;
59 bytes, text/x-review-board-request
Details
Bug 1329093 - Part 5: Update test expectations;
59 bytes, text/x-review-board-request
Details 
See nsSVGElement::WalkContentStyleRules and the way it produces those rules.  Clearly that's not going to work for stylo as things stand, since stylo doesn't have rulewalkers.

The right way to handle this probably depends on how stylo is handling mapped attributes on HTML.
Servo style currently doesn't handle these things at all; they're relegated to synthesize_presentational_hints_for_legacy_attributes in the DOM on Element. Servo's style system is kept pretty separate from the DOM and doesn't know about these things.

Perhaps these should be handled the same place we set style attributes on the gecko side?
Just to be clear, this bug is for the _gecko_ part of handling this stuff.  The _servo_ part is tracked in bug 1329088.

Style attributes in stylo seem to be implemented as follows:

1)  There is a ServoDeclarationBlock::FromCssText which calls Servo_ParseStyleAttribute,
    which calls (on the servo side) GeckoElement::parse_style_attribute,
    which calls style::properties::parse_style_attribute.  The result of all this is a
    PropertyDeclarationBlock, which is a thing servo's style system does know about.
2)  Servo's style system also knows about inline style, which it queries off Element
    by calling style_attribute() and expecting to get back a PropertyDeclarationBlock.
    We set this up on the Gecko impl of Element via Gecko_GetServoDeclarationBlock.

OK, so for this particular bug...  It looks like synthesize_presentational_hints_for_legacy_attributes is handed a Push<PropertyDeclarationBlock> to write to.  The SVG code, if bug 1329088 is fixed so it has a PropertyDeclarationBlock, could push ApplicableDeclarationBlock::from_declarations into it.  Splitting up exactly which bits here happen in Rust and which in C++ and how to communicate between them would be the hard of this, really.

HTML preshints would need to be done somewhat differently, but that's a separate story from this bug.

As far as the SVG animation rules... we could probably express them as a preshint too.  The xml:lang bits come between the two in Gecko right now, but we could manage that, probably.
Depends on: 1339252
What do you mean by "attendant functionality" here?
A stylo equivalent of nsSVGElement::WalkContentStyleRules.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Comment on attachment 8840233 [details]
Bug 1329093 - Part 1: stylo: Handle SVG presentation attributes;

https://reviewboard.mozilla.org/r/114710/#review116198

::: layout/style/ServoBindings.cpp:359
(Diff revision 1)
>    return reinterpret_cast<const RawServoDeclarationBlockStrong*>
>      (decl->AsServo()->RefRaw());
>  }
>  
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetHTMLPresentationAttrDeclarationBlock(RawGeckoElementBorrowed aElement)

Given that this is no longer just HTML, I'm wondering if I should rename this or create a second function for SVG.
Pushing up the WIP. This works and could be landed as-is (post review) with followups on being lazier with the calculations (also handling the URL stuff), so that we can get the bulk of the svg-based reftests passing. Or we could try and get everything done here, your choice.

Currently, this eagerly calculates a ServoDeclarationBlock each time an SVG attribute is changed. We probably should be doing this lazily by storing some kind of list of to-be-resolved SVG elements on the styleset or something. I'm not yet sure what we should be doing here.

This also doesn't know how to handle the special parsing mode for SVG, which we can do in a followup (or in the same bug, really, depends on how much time I have on my hands before this gets reviewed)

mozreview isn't letting me r?bz, so ni?ing instead. Feel free to redirect to someone else if you're busy.
Comment on attachment 8840233 [details]
Bug 1329093 - Part 1: stylo: Handle SVG presentation attributes;

https://reviewboard.mozilla.org/r/114710/#review116644

This seems to be on the right track, but see the comments below about this likely not being quite the right API shape.

::: dom/svg/nsSVGElement.h:328
(Diff revision 1)
>  
>    virtual bool IsSVGFocusable(bool* aIsFocusable, int32_t* aTabIndex);
>    virtual bool IsFocusableInternal(int32_t* aTabIndex, bool aWithMouse) override;
>  
> +  void UpdateContentDeclarationBlock(mozilla::StyleBackendType aBackend);
> +  RefPtr<mozilla::DeclarationBlock> GetContentDeclarationBlock();

Gecko style is to return a mozilla::DeclarationBlock* here.  The caller can then take a ref if they really need one.

Also, this should be a const method.

::: dom/svg/nsSVGElement.cpp:306
(Diff revision 1)
>    // the content declaration block.
>    // XXX For some reason incremental mapping doesn't work, so for now
>    // just delete the style rule and lazily reconstruct it as needed).
>    if (aNamespaceID == kNameSpaceID_None && IsAttributeMapped(aName)) {
>      mContentDeclarationBlock = nullptr;
> +    // TODO we should be doing this lazily by caching these on the styleset

I can live with this given that we're limiting this to the servo case.  But please make sure a bug to fix this is filed and blocking shipping, because we definitely do not want to ship this.

::: dom/svg/nsSVGElement.cpp:1255
(Diff revision 1)
> +    if (mBackend == StyleBackendType::Gecko) {
> -    mParser.ParseProperty(propertyID, aMappedAttrValue, mDocURI, mBaseURI,
> +      mParser.ParseProperty(propertyID, aMappedAttrValue, mDocURI, mBaseURI,
> -                          mElement->NodePrincipal(), mDecl, &changed, false, true);
> +                            mElement->NodePrincipal(), mDecl->AsGecko(), &changed, false, true);
> +    } else {
> +      NS_ConvertUTF16toUTF8 value(aMappedAttrValue);
> +      // TODO pass in the base/principal, update CSSOM to do this as well

Again, please make sure a bug is filed for this.  I wonder whether this is the same underlying issue as bug 1341690, really.

::: dom/svg/nsSVGElement.cpp:1256
(Diff revision 1)
> -    mParser.ParseProperty(propertyID, aMappedAttrValue, mDocURI, mBaseURI,
> +      mParser.ParseProperty(propertyID, aMappedAttrValue, mDocURI, mBaseURI,
> -                          mElement->NodePrincipal(), mDecl, &changed, false, true);
> +                            mElement->NodePrincipal(), mDecl->AsGecko(), &changed, false, true);
> +    } else {
> +      NS_ConvertUTF16toUTF8 value(aMappedAttrValue);
> +      // TODO pass in the base/principal, update CSSOM to do this as well
> +      changed = Servo_DeclarationBlock_SetPropertyById(mDecl->AsServo()->Raw(), propertyID, &value, false);

This will definitely need an SVG mode thing for unitless lengths.  Followup is OK; make sure it's filed.

::: dom/svg/nsSVGElement.cpp:1419
(Diff revision 1)
>    if (!doc) {
>      NS_ERROR("SVG element without owner document");
>      return;
>    }
>  
>    MappedAttrParser mappedAttrParser(doc->CSSLoader(), doc->GetDocumentURI(),

We need to fix this too; otherwise SMIL stuff won't work right.  That is, an SVG element has _two_ content declaration blocks: the one from mapped attributes and the one from SMIL-animated attributes.

Followup is probably OK here too, but this somewhat affects the API shape around this stuff.

::: layout/style/ServoBindings.cpp:359
(Diff revision 1)
>    return reinterpret_cast<const RawServoDeclarationBlockStrong*>
>      (decl->AsServo()->RefRaw());
>  }
>  
>  RawServoDeclarationBlockStrongBorrowedOrNull
>  Gecko_GetHTMLPresentationAttrDeclarationBlock(RawGeckoElementBorrowed aElement)

Given that SVG will need to hand back two declaration blocks, we may want a separate SVG thing....

::: layout/style/ServoBindings.cpp:366
(Diff revision 1)
>    static_assert(sizeof(RefPtr<RawServoDeclarationBlock>) ==
>                  sizeof(RawServoDeclarationBlockStrong),
>                  "RefPtr should just be a pointer");
>    const nsMappedAttributes* attrs = aElement->GetMappedAttributes();
>    if (!attrs) {
> +    auto* svg = nsSVGElement::FromContentOrNull(const_cast<dom::Element*>(aElement));

The cast business is rather worrisome.  Maybe a followup to make the FromContent stuff output both const and non-const overloads?

::: layout/style/ServoBindings.cpp:368
(Diff revision 1)
>                  "RefPtr should just be a pointer");
>    const nsMappedAttributes* attrs = aElement->GetMappedAttributes();
>    if (!attrs) {
> +    auto* svg = nsSVGElement::FromContentOrNull(const_cast<dom::Element*>(aElement));
> +    if (svg) {
> +      if (auto decl = svg->GetContentDeclarationBlock()) {

And having GetContentDeclarationBlock() return a weak ref would be more honest here: we're going to borrow the thing, and then _return_ it.  This is only ok because `svg` holds on to it, not because GetContentDeclarationBlock() returns a strong ref.

::: layout/style/ServoBindings.cpp:370
(Diff revision 1)
>    if (!attrs) {
> +    auto* svg = nsSVGElement::FromContentOrNull(const_cast<dom::Element*>(aElement));
> +    if (svg) {
> +      if (auto decl = svg->GetContentDeclarationBlock()) {
> +        const RefPtr<RawServoDeclarationBlock>& servo = decl->AsServo()->RawRefPtr();
> +        return reinterpret_cast<const RawServoDeclarationBlockStrong*>(&servo);

Having to reinterpret_cast here is really icky too.  Especially if we do it differently in different places.  Compare this code to what Gecko_GetStyleAttrDeclarationBlock does.

How about we add a method returning `const RawServoDeclarationBlockStrong*` on  ServoDeclarationBlock and centralize all the casting?

::: layout/style/ServoDeclarationBlock.h:41
(Diff revision 1)
>                    sizeof(RawServoDeclarationBlock*),
>                    "RefPtr should just be a pointer");
>      return reinterpret_cast<RawServoDeclarationBlock* const*>(&mRaw);
>    }
>  
> +  const RefPtr<RawServoDeclarationBlock>& RawRefPtr() const

This can go away if we centralize the casting, right?
Comment on attachment 8840233 [details]
Bug 1329093 - Part 1: stylo: Handle SVG presentation attributes;

https://reviewboard.mozilla.org/r/114710/#review116644

> Again, please make sure a bug is filed for this.  I wonder whether this is the same underlying issue as bug 1341690, really.

It is, and I already fixed it; but I need to clean up and push.
Blocks: 1341690
Blocks: 1342557
Blocks: 1342559
Comment on attachment 8840233 [details]
Bug 1329093 - Part 1: stylo: Handle SVG presentation attributes;

https://reviewboard.mozilla.org/r/114710/#review116644

> The cast business is rather worrisome.  Maybe a followup to make the FromContent stuff output both const and non-const overloads?

Fixed it in a separate commit.
Wasn't able to finish doing the caching over the weekend, will do this week. In the meantime, r?
Flags: needinfo?(bzbarsky)
Added caching.
try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aa042a72de34124cc2ba18f9a025741792ea164

the one assertion was fixed by bug 1340257, so this is ready to land once it's reviewed.
Comment on attachment 8841098 [details]
Bug 1329093 - Part 3: Overload FromContent() to work with const;

https://reviewboard.mozilla.org/r/115436/#review118312

r=me
Attachment #8841098 - Flags: review+
Comment on attachment 8840233 [details]
Bug 1329093 - Part 1: stylo: Handle SVG presentation attributes;

https://reviewboard.mozilla.org/r/114710/#review118292

r=me.

::: dom/svg/nsSVGElement.cpp:1369
(Diff revisions 1 - 4)
>  }
>  
> -RefPtr<DeclarationBlock>
> -nsSVGElement::GetContentDeclarationBlock()
> +const DeclarationBlock*
> +nsSVGElement::GetContentDeclarationBlock() const
>  {
> -  return mContentDeclarationBlock;
> +  return mContentDeclarationBlock.get();

Why do you need the .get()?  Does the existing conversion operator on RefPtr not do what you want?
Attachment #8840233 - Flags: review+
Comment on attachment 8840802 [details]
Bug 1329093 - Part 2: stylo: Pass parser URL data in Servo_DeclarationBlock_SetProperty*;

https://reviewboard.mozilla.org/r/115236/#review118316

r=me modulo the comments below.  Please note the part that I'm not comfortable reviewing.

::: dom/animation/KeyframeUtils.cpp:1027
(Diff revision 3)
> -      new ThreadSafeURIHolder(aDocument->GetDocumentURI());
> -    RefPtr<ThreadSafePrincipalHolder> principal =
> -      new ThreadSafePrincipalHolder(aDocument->NodePrincipal());
>  
>      nsCString baseString;
> +    GeckoParserExtraData data(aDocument->GetDocumentURI(),

Gah.  I know you just preserved the existing code, but that code is wrong.  On both the gecko and servo branches.  :(

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1343919

::: dom/svg/nsSVGElement.cpp:1256
(Diff revision 3)
>        mParser.ParseProperty(propertyID, aMappedAttrValue, mDocURI, mBaseURI,
>                              mElement->NodePrincipal(), mDecl->AsGecko(), &changed, false, true);
>      } else {
>        NS_ConvertUTF16toUTF8 value(aMappedAttrValue);
> -      // TODO pass in the base/principal, update CSSOM to do this as well
> +      nsCString baseString;
> +      GeckoParserExtraData data(mBaseURI, mBaseURI, mElement->NodePrincipal());

The "sheet uri" (second arg to GeckoParserExtraData) should not be mBaseURI.  It should be mDocURI.

That said, is the second arg really "the referrer"?  Or is it used for anything else?  That is, do we need separate args for "referrer URI" and "stylesheet URI" or whatever?  The two may not match in various cases.  Right now Gecko doesn't have a distinction between the two, but that's a bug that really needs fixing and I've been working on it on and off...

Anyway, having two separate args, if it might be needed, should be a followup.  But please do fix what you're passing here.

::: dom/svg/nsSVGElement.cpp:1257
(Diff revision 3)
>                              mElement->NodePrincipal(), mDecl->AsGecko(), &changed, false, true);
>      } else {
>        NS_ConvertUTF16toUTF8 value(aMappedAttrValue);
> -      // TODO pass in the base/principal, update CSSOM to do this as well
> +      nsCString baseString;
> +      GeckoParserExtraData data(mBaseURI, mBaseURI, mElement->NodePrincipal());
> +      mBaseURI->GetSpec(baseString);

This is a horrible performance and crashiness pitfall.  If you have a data: SVG (which is pretty uncommon), then suddently any pres attr requires making a copy of the entire SVG text here.  Even in the (likely, given it's a data: url!) case that it doesn't need a base URL at all.

Please make sure a bug blocking stylo is filed on this.  We need to figure out something better here....

::: layout/style/nsDOMCSSDeclaration.cpp:309
(Diff revision 3)
>      cssParser.ParseProperty(aPropID, aPropValue,
>                              env.mSheetURI, env.mBaseURI, env.mPrincipal,
>                              decl->AsGecko(), &changed, aIsImportant);
>    } else {
>      NS_ConvertUTF16toUTF8 value(aPropValue);
> +    GeckoParserExtraData data(env.mBaseURI, env.mBaseURI, env.mPrincipal);

Again, the second arg should be env.mSheetURI, not env.mBaseURI, right?

Do we not have any test coverage for this stuff for cases when the document URI and base URI don't match?  :(

::: layout/style/nsDOMCSSDeclaration.cpp:311
(Diff revision 3)
>                              decl->AsGecko(), &changed, aIsImportant);
>    } else {
>      NS_ConvertUTF16toUTF8 value(aPropValue);
> +    GeckoParserExtraData data(env.mBaseURI, env.mBaseURI, env.mPrincipal);
> +    nsCString baseString;
> +    env.mBaseURI->GetSpec(baseString);

This, again, is a nasty performance pitfall.  If I'm banging on some element's style.top in script, why should I need to pay the cost of this GetSpec call?

The only good news is that common cases in which style.top is set the GetSpec will share a buffer, so at least won't involve too much copying.  But if we want to try to solve this for the SVG case, this one can use the same setup for free, I expect.

::: layout/style/nsDOMCSSDeclaration.cpp:359
(Diff revision 3)
>                              env.mBaseURI, env.mPrincipal, decl->AsGecko(),
>                              &changed, aIsImportant);
>    } else {
>      NS_ConvertUTF16toUTF8 property(aPropertyName);
>      NS_ConvertUTF16toUTF8 value(aPropValue);
> +    GeckoParserExtraData data(env.mBaseURI, env.mBaseURI, env.mPrincipal);

Second arg should be env.mSheetURI.

::: servo/components/style/gecko_bindings/structs_debug.rs:25700
(Diff revision 3)
> +        pub mReferrer: root::RefPtr<root::nsMainThreadPtrHolder<root::nsIURI>>,
> +        pub mPrincipal: root::RefPtr<root::nsMainThreadPtrHolder<root::nsIPrincipal>>,
> +    }
> +    #[test]
> +    fn bindgen_test_layout_GeckoParserExtraData() {
> +        assert_eq!(::std::mem::size_of::<GeckoParserExtraData>() , 24usize ,

This is going to fail on 32-bit, right?  I guess that's good in that it will make us think about it then.... ;)

::: servo/components/style/parser.rs:50
(Diff revision 3)
>  
> +#[cfg(feature="gecko")]
> +impl ParserContextExtraData {
> +    /// Construct from a GeckoParserExtraData
> +    pub unsafe fn new(data: *const ::gecko_bindings::structs::GeckoParserExtraData) -> Self {
> +        unsafe { ParserContextExtraData {

I don't know the pieces here well enough to review this part.  Need either more information or another reviewer.

In particular, is it safe to not take refs (which is what I think this code does; I see no GeckoArc* here), because we assume the GeckoParserExtraData will outlive our call?
Attachment #8840802 - Flags: review+
Comment on attachment 8842639 [details]
Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later;

https://reviewboard.mozilla.org/r/115916/#review118330

::: dom/svg/nsSVGElement.cpp:307
(Diff revision 2)
>    // just delete the style rule and lazily reconstruct it as needed).
>    if (aNamespaceID == kNameSpaceID_None && IsAttributeMapped(aName)) {
>      mContentDeclarationBlock = nullptr;
> -    // TODO we should be doing this lazily by caching these on the styleset
>      if (OwnerDoc()->GetStyleBackendType() == StyleBackendType::Servo) {
> -      UpdateContentDeclarationBlock(StyleBackendType::Servo);
> +      auto presContext = OwnerDoc()->GetShell()->GetPresContext();

Nothing guarantees OwnerDoc()->GetShell() is non-null here.

As a simple example, consider the case when OwnerDoc() comes from `new DOMParser().parseFromString(stuff)`.  If we have no test coverage for this, please add some.

What you probably want to do is just store this stuff on the document itself.  Even if we hve a style set right now, it could go away and a new one get created before the next time we compute style, in which case our "we need to recompute" info will be lost.

Also, we need to handle the element being adopted into a different document...  Again, need tests.
Attachment #8842639 - Flags: review-
Comment on attachment 8842640 [details]
Bug 1329093 - Part 5: Update test expectations;

https://reviewboard.mozilla.org/r/116406/#review118332

This is mostly awesome-looking.  r+, modulo the three questions below:

::: dom/base/crashtests/crashtests.list:54
(Diff revision 2)
>  load 377960-2.html
>  load 384663-1.html
>  load 386000-1.html
>  load 386794-1.html
>  skip-if(stylo) load 387460-1.html # bug 1323647
> -asserts-if(stylo,2) load 395469-1.xhtml # bug 1324704
> +asserts-if(stylo,3) load 395469-1.xhtml # bug 1324704

Are the new asserts here really still bug 1324704 or a different bug?

::: layout/style/test/stylo-failures.md:401
(Diff revision 2)
>    * whitespace should be required around '+'/'-' servo/servo#15486
>      * test_property_syntax_errors.html `calc(2em+` [6]
>      * ... `calc(50%+` [8]
>    * &lt;position&gt; value accepts invalid 3-value forms servo/servo#15488
>      * test_property_syntax_errors.html `'background-position'` [12]
> -    * ... `mask-position'` [20]
> +    * ... `mask-position'` [24]

Why are there more failures here?  Did some of them just use to get picked up by earlier things in the file that are being removed?  Or ar these new failures?

::: layout/style/test/stylo-failures.md:529
(Diff revision 2)
>      * ... `hue-rotate(0)` [6]
>  * test_moz_device_pixel_ratio.html: probably unship -moz-device-pixel-ratio bug 1338425 [4]
>  
>  ## Spec Unclear
>  
> -* test_property_syntax_errors.html `'background'`: whether background shorthand should accept "text" [40]
> +* test_property_syntax_errors.html `'background'`: whether background shorthand should accept "text" [200]

Again, why the jump in number of failures here?  Are we ending up with things that used to be picked up by the more specific earlier annotations?
Attachment #8842640 - Flags: review+
Comment on attachment 8840802 [details]
Bug 1329093 - Part 2: stylo: Pass parser URL data in Servo_DeclarationBlock_SetProperty*;

https://reviewboard.mozilla.org/r/115236/#review118316

> The "sheet uri" (second arg to GeckoParserExtraData) should not be mBaseURI.  It should be mDocURI.
> 
> That said, is the second arg really "the referrer"?  Or is it used for anything else?  That is, do we need separate args for "referrer URI" and "stylesheet URI" or whatever?  The two may not match in various cases.  Right now Gecko doesn't have a distinction between the two, but that's a bug that really needs fixing and I've been working on it on and off...
> 
> Anyway, having two separate args, if it might be needed, should be a followup.  But please do fix what you're passing here.

I don't think we need two separate args here, it's just called "referrer" in the existing API (heycam would know more)

> Again, the second arg should be env.mSheetURI, not env.mBaseURI, right?
> 
> Do we not have any test coverage for this stuff for cases when the document URI and base URI don't match?  :(

We might have coverage, the problem is that we still have lots of tests failing for other reasons so it's hard to tell.

> This is going to fail on 32-bit, right?  I guess that's good in that it will make us think about it then.... ;)

Yes, all of our checked in bindings are 64 bit only for now. This file is only used whilst testing on the Servo side (and will be removed when the CI work is complete).

Build time bindgen will override this file (but it is supposed to produce approximately equivalent results). It should work in 32 bit, but if it doesn't that's a problem for later when we have CI.

> I don't know the pieces here well enough to review this part.  Need either more information or another reviewer.
> 
> In particular, is it safe to not take refs (which is what I think this code does; I see no GeckoArc* here), because we assume the GeckoParserExtraData will outlive our call?

`data` is a ref to a struct containing geckoarcs. We assume it outlives the call. `to_safe` addrefs and makes a safe servo-side refptr out of it.
Comment on attachment 8842640 [details]
Bug 1329093 - Part 5: Update test expectations;

https://reviewboard.mozilla.org/r/116406/#review118332

> Are the new asserts here really still bug 1324704 or a different bug?

The same one, I think. At least, that's what I got when I ran it manually.

> Why are there more failures here?  Did some of them just use to get picked up by earlier things in the file that are being removed?  Or ar these new failures?

Probably due to the `mask` test not being marked correctly, I should have used `'mask'` as the pattern; its failures probably shifted around. I'll fix.

> Again, why the jump in number of failures here?  Are we ending up with things that used to be picked up by the more specific earlier annotations?

No, this is because relative urls used to make background parsing fail, but now they don't, so actual bugs in our parsing being too liberal were masked by relative urls.
Flags: needinfo?(bzbarsky)
Comment on attachment 8842639 [details]
Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later;

https://reviewboard.mozilla.org/r/115916/#review118728

::: dom/base/nsDocument.h:802
(Diff revision 4)
>    virtual void NotifyIntersectionObservers() override;
>  
>    virtual void NotifyLayerManagerRecreated() override;
>  
> +  virtual void ScheduleSVGForPresAttrEvaluation(nsSVGElement* aSVG) override;
> +  virtual void ResolveScheduledSVGPresAttrs() override;

So here's a problem.  Nothing guarantees that this function will ever be called.  The document could be a data document and hence never even get a styleset, or it could be in a display:none iframe.

And if that happens, we will leak, because the document is holding strong refs to the elements but not cycle-collecting them, and of course elements hold strong refs to documents.

This could even leak, though only for document lifetime, even if we _were_ cycle-collecting.  Think a page that creates an SVG element, sets some attributes, then just forgets about it.  It will hang out in this hashtable forever until the document as a whole goes away.

Maybe what we should do is hold weak refs here via nsWeakPtr and clear things out every so often or something.  Or hold raw pointer refs and remove on element destruction or adoption out of the document...

One more thing: we probably need to call ResolveScheduledSVGPresAttrs when people ask for computed style too.  Should be pretty simple to write a testcase which fails without doing that.
Attachment #8842639 - Flags: review-
Comment on attachment 8842639 [details]
Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later;

https://reviewboard.mozilla.org/r/115916/#review118914

There is still one critical piece missing, afaict: you need to unschedule on adopt.  Otherwise the element will not be able to remove itself from the old document (because now it has a new OwnerDoc()) and we will get a crash.  The simplest thing is probably to change Element::NodeInfoChanged to take an nsIDocument* aOldDoc and pass "oldDoc" to it at the one callsite.  Then implement NodeInfoChanged on nsSVGElement, remove from oldDoc, and probably add to the OwnerDoc() (which is the new doc) at that point.  Should be able to write a crashtest for this.

::: dom/base/nsDocument.h:1640
(Diff revision 5)
>    // Used to prevent multiple requests to ServiceWorkerManager.
>    bool mMaybeServiceWorkerControlled;
>  
> +  // We lazily calculate declaration blocks for SVG elements
> +  // with mapped attributes in Servo mode. This list contains all elements which
> +  // need lazy resolution

Please describe here the ownership model, aka why it's ok to hold weak refs here.

::: dom/base/nsDocument.cpp:6025
(Diff revision 5)
> +}
> +
> +void
> +nsDocument::UnscheduleSVGForPresAttrEvaluation(nsSVGElement* aSVG)
> +{
> +  if (auto entry = mLazySVGPresElements.GetEntry(aSVG)) {

Why not just:

    mLazySVGPresElements.RemoveEntry(aSVG);
    
?  The form where you get and then remove using the entry pointer is for cases when you want to do something with the entry before removing.

::: dom/svg/crashtests/1329093-1.html:5
(Diff revision 5)
> +<html class="reftest-wait">
> +
> +<head>
> +<script>
> +setTimeout('document.documentElement.className = ""', 500);

Why do you need this?  I'd think just waiting for the load event would be enough.  As in, no need for either the reftest-wait bit nor the script at all...
Attachment #8842639 - Flags: review-
Comment on attachment 8842639 [details]
Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later;

https://reviewboard.mozilla.org/r/115916/#review118914

> Why not just:
> 
>     mLazySVGPresElements.RemoveEntry(aSVG);
>     
> ?  The form where you get and then remove using the entry pointer is for cases when you want to do something with the entry before removing.

The docs mentioned that RemoveEntry is supposed to be used with the result of GetEntry. I thought that `GetEntry` may be returning something different from the original pointer, e.g. a pointer to the hashtable slot.

> Why do you need this?  I'd think just waiting for the load event would be enough.  As in, no need for either the reftest-wait bit nor the script at all...

The other iframe test in the same directory does this.
> The docs mentioned that RemoveEntry is supposed to be used with the result of GetEntry.

There are two overloads of RemoveEntry.  One takes an Entry*, one takes a key (in this case aSVG).

> The other iframe test in the same directory does this.

Right, but its iframe is itself running things off timers.  Though hilariously the iframe runs things off a 1000ms timer and the test finishes off a 500ms timer, so it's not testing anything.  Should really fix the test to use a 250ms timer in the subframe or something....  Or better yet, no timer for the main test, just a callback from the subframe once it's done.  Totally out of the scope of this bug, though.
Ready for review.

The crashtest doesn't actually crash pre-part-6.  Not sure how to improve it so that it does (perhaps force a style recalc somewhere).

An interesting side bug (in gecko proper, not stylo) I discovered here is that if I have an svg element containing a circle and a filter, and the circle uses the filter via url(#foo), adopting the entire svg element will give an invisible circle. The circle still exists according to devtools, and removing the filter makes it visible again, but the filter filters everything out otherwise. Not entirely sure if this is a bug.
Flags: needinfo?(bzbarsky)
> Not entirely sure if this is a bug.

It's not.  SVG defines that if you reference a paint server that doesn't exist, you don't get painted.
Comment on attachment 8842639 [details]
Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later;

https://reviewboard.mozilla.org/r/115916/#review120366

::: dom/base/nsDocument.h:1641
(Diff revision 6)
>    bool mMaybeServiceWorkerControlled;
>  
> +  // We lazily calculate declaration blocks for SVG elements
> +  // with mapped attributes in Servo mode. This list contains all elements which
> +  // need lazy resolution
> +  nsTHashtable<nsPtrHashKey<nsSVGElement> > mLazySVGPresElements;

You don't need the space in "> >".
Attachment #8842639 - Flags: review-
Comment on attachment 8844689 [details]
Bug 1329093 - Part 6: stylo: Handle cross-document node adoption in scheduled SVG content decl blocks;

https://reviewboard.mozilla.org/r/118006/#review120370

r=me with the nits below.  Making the test work reliably might be a bit painful, because you need to make sure the element dies, even though we've got GC and CC nondeterminism.  :(

::: dom/base/Element.h:1114
(Diff revision 1)
>     * is, this should only be called from methods that only care about attrs
>     * that effectively live in mAttrsAndChildren.
>     */
>    virtual BorrowedAttrInfo GetAttrInfo(int32_t aNamespaceID, nsIAtom* aName) const;
>  
> -  virtual void NodeInfoChanged()
> +  virtual void NodeInfoChanged(nsIDocument* aOldDoc)

Toss in a comment about how this is called right after the element has been adopted into a new document, and that the new document can be reached via OwnerDoc()?

::: dom/svg/crashtests/1329093-2.html:16
(Diff revision 1)
> +
> +<iframe src="" id="myFrame"></iframe>
> +<script type="text/javascript">
> +let frame = document.getElementById("myFrame");
> +frame.onload = function() {
> +    let baz = frame.contentDocument.adoptNode(document.getElementById("svgElement"));

What I think you want to do for this test to really be testing things is after that adoptNode call you want to drop your reference to the element (e.g. by doing `baz = null`, do frame.remove(), do `frame = null`, and then trigger some GC/CC to actually kill off the element.  Then trigger a restyle on the parent document.  Or even just call getComputedStyle() on something in the parent document that has no frame and ask for its .color; that should trigger the relevant codepath, I think.

::: dom/svg/nsSVGElement.h:118
(Diff revision 1)
>                                                int32_t aModType) const override;
>  
>    virtual bool IsNodeOfType(uint32_t aFlags) const override;
>  
> +  /**
> +   * Called when we have been cloned and adopted, and the information of the

Yeah, so this comment should be in Element.h  ;)

And s/cloned and adopted/adopted/; the cloning part is not relevant.

::: dom/svg/nsSVGElement.h:121
(Diff revision 1)
>  
> +  /**
> +   * Called when we have been cloned and adopted, and the information of the
> +   * node has been changed.
> +   *
> +   * We override it to unschedule computation of Servo declaration blocks

But this part can stay here, obviously.
Attachment #8844689 - Flags: review+
Comment on attachment 8844689 [details]
Bug 1329093 - Part 6: stylo: Handle cross-document node adoption in scheduled SVG content decl blocks;

https://reviewboard.mozilla.org/r/118006/#review120392

::: dom/svg/crashtests/1329093-2.html:22
(Diff revisions 1 - 2)
>      let baz = frame.contentDocument.adoptNode(document.getElementById("svgElement"));
>      frame.contentDocument.body.appendChild(baz);
> +    baz = null;
> +    frame.remove();
> +    frame = null;
> +    let color = getComputedStyle(document.getElementById('triggerRestyle')).color;

This part needs to happen _after_ the GC, right?  The whole point is that when we go to compute the style we'll create a new style context, which will try to update styles on SVG things, and if we're holding a ref to a dead one...

::: dom/svg/crashtests/1329093-2.html:23
(Diff revisions 1 - 2)
>      frame.contentDocument.body.appendChild(baz);
> +    baz = null;
> +    frame.remove();
> +    frame = null;
> +    let color = getComputedStyle(document.getElementById('triggerRestyle')).color;
> +    document.getElementById('triggerRestyle').remove(); // try to trigger gc

How about:

  SpecialPowers.gc();
  
instead?  That might be more reliable.  Might even be reliable enough for you to reproduce the crash if you comment out the remove in the nodeinfochanged function, I hope.
Attachment #8844689 - Flags: review+
Comment on attachment 8842639 [details]
Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later;

https://reviewboard.mozilla.org/r/115916/#review120400

r=me, though ideally part 6 would just get merged into here.
Attachment #8842639 - Flags: review+
Attachment #8844689 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s cf1030ef7e05 -d cf2b0421dd57: rebasing 381287:cf1030ef7e05 "Bug 1329093 - Part 1: stylo: Handle SVG presentation attributes; r=bz"
rebasing 381288:ba1d36bca880 "Bug 1329093 - Part 2: stylo: Pass parser URL data in Servo_DeclarationBlock_SetProperty*; r=bz"
rebasing 381289:8ce26ffe66dc "Bug 1329093 - Part 3: Overload FromContent() to work with const; r=bz"
rebasing 381290:a3ebeca95a2e "Bug 1329093 - Part 4: stylo: Delay SVG mapped attr resolution till later; r=bz"
rebasing 381291:471ca69b5678 "Bug 1329093 - Part 5: Update test expectations; r=bz" (tip)
merging layout/style/test/stylo-failures.md
warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #8)
> We need to fix this too; otherwise SMIL stuff won't work right.  That is, an
> SVG element has _two_ content declaration blocks: the one from mapped
> attributes and the one from SMIL-animated attributes.

I'm hoping we can get rid of that second declaration block in bug 1062106. That is, I'm hoping we can get rid of the animated mapped attributes case altogether. I have a try run that looks encouraging.

We'll still need the SMIL override stylesheet however (whose declaration blocks are stored in DOM slots, not node properties).
Attachment #8840802 - Attachment is obsolete: true
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/281614c5ea9f
Part 1: stylo: Handle SVG presentation attributes; r=bz
https://hg.mozilla.org/integration/autoland/rev/0d2f87e66db2
Part 3: Overload FromContent() to work with const; r=bz
https://hg.mozilla.org/integration/autoland/rev/33e225044544
Part 4: stylo: Delay SVG mapped attr resolution till later; r=bz
https://hg.mozilla.org/integration/autoland/rev/610c74da6340
Part 5: Update test expectations; r=bz
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: