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

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bz, Assigned: bytesized)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 8859447 [details]
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.
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.
Priority: -- → P3
Attachment #8859447 - Attachment description: Microbenchmark → Microbenchmark, but not for this bug, really, because for the style attribute we already do a no-reparsing fast path.
Attachment #8859447 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Assignee: nobody → ksteuber
(Assignee)

Updated

a year ago
Depends on: 1363481
(Assignee)

Updated

a year ago
Depends on: 1365092
(Assignee)

Updated

a year ago
No longer depends on: 1363481
This blocks a qf:p1 bug so I'm also marking it as qf:p1.
Whiteboard: [qf:p3] → [qf:p1]
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8859448 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8878095 - Flags: review?(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.
Attachment #8878095 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1373744
(Assignee)

Comment 12

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 14

a year ago
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c956b50faeb3
Clone attributes rather than reparsing when possible r=bz

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c956b50faeb3
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
status-firefox55: affected → wontfix
You need to log in before you can comment on or make changes to this bug.