1357645 - Consider a faster path that uses preparsed attributes in nsGenericHTMLElement::CopyInnerTo

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Right now we ToString all the attributes and then reparse them.  We would just use SetParsedAttr instead to copy over the values, sort of like this, in the case when OwnerDoc() == aDst->OwnerDoc():

  nsresult rv;
  int32_t i, count = GetAttrCount();
  for (i = 0; i < count; ++i) {
    const nsAttrName *name = mAttrsAndChildren.AttrNameAt(i);
    const nsAttrValue *value = mAttrsAndChildren.AttrAt(i);

    nsAttrValue valueCopy(*value);
    rv = aDst->SetParsedAttr(name->NamespaceID(), name->LocalName(),
                             name->GetPrefix(), valueCopy, false);
    NS_ENSURE_SUCCESS(rv, rv);
  }

We can't do this for SVG because the ParseAttribute functions there have all sorts of side-effects.  But for HTML, the only side effects once we fix the bugs blocking this one should be:

1)  The class/id stuff that Element::ParseAttribute does.  This could be moved out of Element::ParseAttribute to someplace shared by SetAttr and SetParsedAttr, or we could simply manually set the MayHaveClass/HasID flags in CopyInnerTo.  The thing being copied to is not in a document, so doesn't need to be removed or added to ID tables.

2) The SetMayHaveStyle() call nsStyledElement::ParseAttribute makes.  We could move this to an AfterSetAttr override, or do it manually in CopyInnerTo as needed.

3) The name stuff in nsGenericHTMLElement::ParseAttribute.  This is similar to the class/id situation.

So if we handle those, we could try using the code above.  I tried creating a microbenchmark for this, but didn't see an obvious improvement, unfortunately.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Microbenchmark, but not for this bug, really, because for the style attribute we already do a no-reparsing fast path.

Did some more measurement, and this does improve noticeably (15-20%) with a hacky fix for this bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Hacky and totally not correct patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

When fixing this, we should add a testcase that exercises the "changing owner doc" case (using importNode) using something like "background" or "style" as the attribute.  Should be detectable when the two documents have different base URIs.

Comment 4

[Robin Steuber (she/her) [:bytesized]]

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

Comment 5

[Robin Steuber (she/her) [:bytesized]]

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

Comment 6

[Andrew Overholt [:overholt]]

This blocks a qf:p1 bug so I'm also marking it as qf:p1.

Comment 7

[Robin Steuber (she/her) [:bytesized]]

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

Comment 8

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1357645 - Clone attributes rather than reparsing when possible

Now that the side effects of parsing have been relocated to BeforeSetAttr and AfterSetAttr (Bug 1365092), we can easily switch nsGenericHTMLElement::CopyInnerTo from reparsing attributes to cloning them. They are still reparsed, however, in the case where the owning document is changing since Base URIs may have changed.

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

Comment 9

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8878095 [details]
Bug 1357645 - Clone attributes rather than reparsing when possible

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8878095 [details]
Bug 1357645 - Clone attributes rather than reparsing when possible

https://reviewboard.mozilla.org/r/149502/#review154106

::: dom/html/nsGenericHTMLElement.cpp:185
(Diff revision 2)
> +  MOZ_ASSERT(!aDst->GetUncomposedDoc(),
> +             "Should not CopyInnerTo an Element in a document");
>    nsresult rv;
>  
> +  bool reparse = false;
> +  if (aDst->OwnerDoc() != OwnerDoc()) {

Why not just:

    bool reparse = (aDst->OwnerDoc() != OwnerDoc());
  
?

::: dom/html/nsGenericHTMLElement.cpp:201
(Diff revision 2)
>      const nsAttrName *name = mAttrsAndChildren.AttrNameAt(i);
>      const nsAttrValue *value = mAttrsAndChildren.AttrAt(i);
>  
> +    if (name->Equals(nsGkAtoms::style, kNameSpaceID_None) &&
> +        value->Type() == nsAttrValue::eCSSDeclaration) {
> -    nsAutoString valStr;
> +      nsAutoString valStr;

We don't want to ToString() in the style attr case at all, and this valStr isn't even being used, afaict.  Please take this bit out.

What we do maybe want here is a comment about how this is cloning the style even in the cross-document case, with a pointer to https://github.com/w3c/webappsec-csp/issues/212 perhaps.

::: dom/html/nsGenericHTMLElement.cpp:208
(Diff revision 2)
> -        value->Type() == nsAttrValue::eCSSDeclaration) {
>        DeclarationBlock* decl = value->GetCSSDeclarationValue();
>        // We can't just set this as a string, because that will fail
>        // to reparse the string into style data until the node is
>        // inserted into the document.  Clone the Rule instead.
>        RefPtr<DeclarationBlock> declClone = decl->Clone();

I know that's what the existing code did, but can we not use the valueCopy thing here too?  The difference is that the valueCopy business will just addref the declaration block and hold on to it, instead of cloning it...  I guess we still need to ensure that we mark the declaration block as immutable in that situation (which it might not be if it never got cached), so we still need a bit of extra style attr codepath here.

Followup is ok for this bit, just to mitigate risk.

Comment 11

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8878095 [details]
Bug 1357645 - Clone attributes rather than reparsing when possible

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

Comment 12

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8878095 [details]
Bug 1357645 - Clone attributes rather than reparsing when possible

https://reviewboard.mozilla.org/r/149502/#review154106

> We don't want to ToString() in the style attr case at all, and this valStr isn't even being used, afaict.  Please take this bit out.
> 
> What we do maybe want here is a comment about how this is cloning the style even in the cross-document case, with a pointer to https://github.com/w3c/webappsec-csp/issues/212 perhaps.

As discussed, this is still necessary but will be removed in Bug 1373744.

> I know that's what the existing code did, but can we not use the valueCopy thing here too?  The difference is that the valueCopy business will just addref the declaration block and hold on to it, instead of cloning it...  I guess we still need to ensure that we mark the declaration block as immutable in that situation (which it might not be if it never got cached), so we still need a bit of extra style attr codepath here.
> 
> Followup is ok for this bit, just to mitigate risk.

Filed Bug 1373744.

Comment 13

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8878095 [details]
Bug 1357645 - Clone attributes rather than reparsing when possible

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c956b50faeb3
Clone attributes rather than reparsing when possible r=bz

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/c956b50faeb3