Bug 172899 – Streamline handling of attributes, using references as much as possible

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Streamline handling of attributes, using references as much as possible

[Darin Adler]

Created attachment 311944 [details]
Patch

[Build Bot]

Comment on attachment 311944 [details]
Patch

Attachment 311944 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3867812

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311949 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 311959 [details]
Patch

[Darin Adler]

This new version is passing all the tests on EWS.

[Chris Dumez]

Comment on attachment 311959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311959&action=review

r=me with comment

> Source/WebCore/dom/ScriptElement.cpp:320
> +    auto& nonce = m_element.attributeWithoutSynchronization(HTMLNames::nonceAttr);

This looks unsafe given the dispatchBeforeLoadEvent() call below. We execute JS which could modify the attributes and invalidate the reference.

> Source/WebCore/html/HTMLAudioElement.cpp:54
> +    element->setAttributeWithoutSynchronization(preloadAttr, "auto");

ASCIILiteral("auto") ?

[Darin Adler]

Comment on attachment 311959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311959&action=review

Thanks for reviewing.

>> Source/WebCore/dom/ScriptElement.cpp:320
>> +    auto& nonce = m_element.attributeWithoutSynchronization(HTMLNames::nonceAttr);
> 
> This looks unsafe given the dispatchBeforeLoadEvent() call below. We execute JS which could modify the attributes and invalidate the reference.

Glad you caught that. How did I miss it?

>> Source/WebCore/html/HTMLAudioElement.cpp:54
>> +    element->setAttributeWithoutSynchronization(preloadAttr, "auto");
> 
> ASCIILiteral("auto") ?

I am torn. My apologies in advance for using { } and spaces instead of () below, I don’t know why I am doing this all the time, except that I like the type strictness.

1) "auto", implicitly turned into AtomicString { "auto" }
2) ASCIILiteral { "auto" }, implicitly turned into AtomicString { ASCIILiteral { "auto" } }
3) AtomicString { "auto", AtomicString::ConstructFromLiteral }

I am pretty sure (2) doesn’t work. See this code from AtomicString.h:

    // The explicit constructors with AtomicString::ConstructFromLiteral must be used for literals.
    AtomicString(ASCIILiteral);

I don’t really understand the tradeoffs here. None of these do any memory allocation if the literal is already in the string table. (3) avoids making a call to strlen or doing the equivalent by checking each character to see if it is null. (2) and (3) avoid copying the characters into the StringImpl if one is allocated. (3) has more code at each call site than (1).

Then there are the preallocation strategies:

4) static NeverDestroyed<AtomicString> autoString { "auto", AtomicString::ConstructFromLiteral };

Ugly when used just for a constant, with the NeverDestroyed and having to figure out what to name it. Why do we do this in the style of (3) rather (1)?

5) A global variable somewhere.

For example, the five special globals in AtomicString.h itself, the ones for all the attribute names in HTMLNames.h. We could add globals for common attribute values as well as attribute names, and if so, I’m sure we would have one for "auto". But would that be a good optimization, or would it just slow things down by spending more time filling the AtomicString hash table with more strings and having a more crowded table.

Given the above, I think I will stick with (1). But maybe you can convince me otherwise.

[Darin Adler]

Committed r217774: <http://trac.webkit.org/changeset/217774>

[Chris Dumez]

Comment on attachment 311959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311959&action=review

>>> Source/WebCore/html/HTMLAudioElement.cpp:54
>>> +    element->setAttributeWithoutSynchronization(preloadAttr, "auto");
>> 
>> ASCIILiteral("auto") ?
> 
> I am torn. My apologies in advance for using { } and spaces instead of () below, I don’t know why I am doing this all the time, except that I like the type strictness.
> 
> 1) "auto", implicitly turned into AtomicString { "auto" }
> 2) ASCIILiteral { "auto" }, implicitly turned into AtomicString { ASCIILiteral { "auto" } }
> 3) AtomicString { "auto", AtomicString::ConstructFromLiteral }
> 
> I am pretty sure (2) doesn’t work. See this code from AtomicString.h:
> 
>     // The explicit constructors with AtomicString::ConstructFromLiteral must be used for literals.
>     AtomicString(ASCIILiteral);
> 
> I don’t really understand the tradeoffs here. None of these do any memory allocation if the literal is already in the string table. (3) avoids making a call to strlen or doing the equivalent by checking each character to see if it is null. (2) and (3) avoid copying the characters into the StringImpl if one is allocated. (3) has more code at each call site than (1).
> 
> Then there are the preallocation strategies:
> 
> 4) static NeverDestroyed<AtomicString> autoString { "auto", AtomicString::ConstructFromLiteral };
> 
> Ugly when used just for a constant, with the NeverDestroyed and having to figure out what to name it. Why do we do this in the style of (3) rather (1)?
> 
> 5) A global variable somewhere.
> 
> For example, the five special globals in AtomicString.h itself, the ones for all the attribute names in HTMLNames.h. We could add globals for common attribute values as well as attribute names, and if so, I’m sure we would have one for "auto". But would that be a good optimization, or would it just slow things down by spending more time filling the AtomicString hash table with more strings and having a more crowded table.
> 
> Given the above, I think I will stick with (1). But maybe you can convince me otherwise.

Oh, I was wrong about using ASCIILiteral() here indeed. I seem to remember Ben asking me to use AtomicString { "auto", AtomicString::ConstructFromLiteral } in such cases though. I do not remember the benefits but I did not look into it in much details. 1) or 3), I do not really mind either way.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.