635832 - Reduce string copying from nsHTMLSharedElement::GetHref

Status: UNCONFIRMED

(Core :: DOM: Core & HTML, defect, P5)

Description

[aliang]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: firefox-4.0b11

After studying the Bug 267506 "Reduce string copying from nsGenericHTMLElement::GetHrefURIForAnchors" at https://bugzilla.mozilla.org/show_bug.cgi?id=267506, I found a similar case in firefox-4.0b11.source/content/html/content/src/nsHTMLSharedElement.cpp:

the "nsAutoString href" is not necessary in function nsHTMLSharedElement::GetHref, and the function can be changed as follows:

// nsIDOMHTMLBaseElement
NS_IMPL_STRING_ATTR(nsHTMLSharedElement, Target, target)
NS_IMETHODIMP
nsHTMLSharedElement::GetHref(nsAString& aValue)
{
-  nsAutoString href;
-  GetAttr(kNameSpaceID_None, nsGkAtoms::href, href);

+  const nsAttrValue* hrefAttrValue =
+    mAttrsAndChildren.GetAttr(nsGkAtoms::href, kNameSpaceID_None);

  nsCOMPtr<nsIURI> uri;
  nsIDocument* doc = GetOwnerDoc();
  if (doc) {
    nsContentUtils::NewURIWithDocumentCharset(
+      getter_AddRefs(uri), hrefAttrValue->GetStringValue(),
+      doc, doc->GetDocumentURI());
-      getter_AddRefs(uri), href, doc, doc->GetDocumentURI());
  }
  if (!uri) {
+    aValue = hrefAttrValue->GetStringValue();
-    aValue = href;
    return NS_OK;
  }
  
  nsCAutoString spec;
  uri->GetSpec(spec);
  CopyUTF8toUTF16(spec, aValue);

  return NS_OK;
}

Reproducible: Didn't try

Comment 1

[Boris Zbarsky [:bzbarsky]]

This isn't performance-sensitive code, though.  And it introduces some fragility, because it assumes things about the type of the href attr, right?

Comment 2

[aliang]

(In reply to comment #1)
> This isn't performance-sensitive code, though.  And it introduces some
> fragility, because it assumes things about the type of the href attr, right?

Can you say more about the fragility? I did not get it. Thanks.

Comment 3

[Boris Zbarsky [:bzbarsky]]

GetStringValue() will assert and crash if the type of the attribute is not eString.  See the implementation in nsAttrValue.cpp.

Comment 4

[Mounir Lamouri (:mounir)]

Hmm, I don't think this is saving any cycles. Whatever happen we will always use string: if |doc| isn't null, we will call |nsContentUtils::NewURIWithDocumentCharset| and if it is, |uri| will be null so we will call |aValue = href|. Even if |href| is used in two different blocks, we will always go in at least one of them, aren't we?

Comment 5

[Boris Zbarsky [:bzbarsky]]

There are costs to constructing the |href| string and possibly to copying data into it and out of it.  So if this were on a hot path, like @href on <a>, we'd want to avoid those costs.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> There are costs to constructing the |href| string and possibly to copying data
> into it and out of it.  So if this were on a hot path, like @href on <a>, we'd
> want to avoid those costs.

That's a cost we just can't avoid given that it seems like we are always using href.

Comment 7

[aliang]

(In reply to comment #3)
> GetStringValue() will assert and crash if the type of the attribute is not
> eString.  See the implementation in nsAttrValue.cpp.

I saw three cases where
    const nsAttrValue* href = mAttrsAndChildren.GetAttr(nsGkAtoms::href,
                                                        kNameSpaceID_XLink);
is used.

So is it because in this case kNameSpaceID_None is used but not kNameSpaceID_XLink, and then hrefAttrValue->GetStringValue() may crash?

In the old bug 267506 patch, kNameSpaceID_None is used there. Does this mean it is also wrong in that patch? If it is, is there any bug report related to this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

It's ok to use the typed getters on nsAttrValue if you know the type of the attribute.

The type of xlink:href is eString (which is the default attribute type).

The worries arise in HTML for various "special" attributes, because some of them are parsed as different types.

In bug 267506, the code is very careful to parse the attribute as the right type.

Comment 9

[aliang]

(In reply to comment #8)
> In bug 267506, the code is very careful to parse the attribute as the right
> type.

Can you say more about how the old code parse the attribute as the right type? I am new to Mozilla code, so I have some trouble to understand this.

I compared nsGenericHTMLElement::GetHrefURIForAnchors() in the latest 4.0b11 version and the version mentioned in the bug 267506, and noticed that in the latest version it is using GetURIAttr(...) rather than GetAttr(...), does this means that whenever nsGenericHTMLElement::GetHrefURIForAnchors() is called, the caller knows the attribute is for URI and thus the type of the attribute?

Thanks.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Can you say more about how the old code parse the attribute as the right type?

Element classes are responsible for parsing their own attributes (see the various ParseAttribute implementations).

> and noticed that in the latest version it is using GetURIAttr(...)

Which actually just does a GetStringValue() on the nsAttrValue.  Anything that wants to use this function needs to make sure to parse as a string.

Comment 11

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.