1451421 - stop registering namespaces in @namespace rules during parsing

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

This is a problem for OMT parsing because the namespace manager isn't threadsafe.

The only reason we do this is so that we can stick the namespace id in the serialization on the style struct, which then gets parsed out by the frame constructor during content generation. There are obviously better ways to do this.

I have patches.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add test for case insensitivity for content: attr() identifiers in html documents. v1

dbaron asked for this in bug 507762, but it never happened.

MozReview-Commit-ID: 6KQKGIAHJ4c

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add an atom overload for namespace registration. v1

MozReview-Commit-ID: 3CiyFImX8UO

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a ToLowerCase utility method for nsAtom. v1

Happy to hear suggestions on how to do this better.

MozReview-Commit-ID: 55KFG1fwCTt

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Add an into_repr on the nsstring bindings. r=mystor

I didn't end up using this, but mystor reviewed it and it's useful.

MozReview-Commit-ID: 5LNzCEGpbLy

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Stop using Gecko namespace ids in servo. v1

MozReview-Commit-ID: 2532dHCGPXW

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8965079 [details] [diff] [review]
Part 1 - Add test for case insensitivity for content: attr() identifiers in html documents. v1

Review of attachment 8965079 [details] [diff] [review]:
-----------------------------------------------------------------

This probably should be a test in wpt.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8965084 [details] [diff] [review]
Part 5 - Stop using Gecko namespace ids in servo. v1

Review of attachment 8965084 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1704,2 @@
>        int32_t attrNameSpace = kNameSpaceID_None;
> +      if (ns) {

nit: you can do
> if (RefPtr<nsAtom> ns = attr->mNamespaceURL) {
>   ...
> }

::: layout/style/nsComputedDOMStyle.cpp
@@ +1386,5 @@
>        case eStyleContentType_Attr: {
> +        // XXXbholley: We don't correctly serialize namespaces here. Doing so
> +        // would require either storing the prefix on the nsStyleContentAttr,
> +        // or poking at the namespaces in the stylesheet to map from the
> +        // namespace URL.

I guess it isn't a big problem to just store prefix in nsStyleContentAttr. Is there any concern for doing that?

::: layout/style/nsStyleStruct.cpp
@@ +4143,5 @@
>    } else if (mType == eStyleContentType_Counter ||
>               mType == eStyleContentType_Counters) {
>      mContent.mCounters->Release();
> +  } else if (mType == eStyleContentType_String) {
> +    MOZ_ASSERT(mContent.mString);

This assertion doesn't seem to be very useful since `free` is a no-op if the pointer is nullptr. Actually, there is no guarantee from the previous code that mContent.mString must have value when mType is eStyleContentType_String.

@@ +4151,1 @@
>    }

Please instead assert here that mContent.mString is null to ensure that we are not leaking anything.

::: servo/components/style/properties/gecko.mako.rs
@@ +5515,1 @@
>                                      },

Hmmm... I really hope we have some sugar for refptr and nsAtom... but I guess this is fine for now.

@@ +5612,5 @@
> +                            let ns = if s.mNamespaceURL.mRawPtr.is_null() {
> +                                None
> +                            } else {
> +                                // FIXME(bholley): We don't have any way to get the prefix here. :-(
> +                                let prefix = Atom::from("");

`atom!("")`.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8965081 [details] [diff] [review]
Part 2 - Add an atom overload for namespace registration. v1

Do you still need the empty-string check in the string overload?  Do we hit that case often?

r=me either way.

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 8965082 [details] [diff] [review]
Part 3 - Add a ToLowerCase utility method for nsAtom. v1

Review of attachment 8965082 [details] [diff] [review]:
-----------------------------------------------------------------

The use cases for this method seem to apply to attribute (and element?) names, but it uses per-code point Unicode case mapping. Attribute and element names should use ASCII-lowercasing (https://infra.spec.whatwg.org/#ascii-lowercase).

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a ToLowerCaseASCII utility method for nsAtom. v2

MozReview-Commit-ID: CcGeRiaQxNW

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a ToLowerCaseASCII utility method for nsAtom. v3

Inverted a boolean condition in the last patch, which the tests caught.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add test for case insensitivity for content: attr() identifiers in html documents. r=xidorn

The test from part 1, rewritten as wpt.

MozReview-Commit-ID: 5W8rOZSGjkk

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Do you still need the empty-string check in the string overload?  Do we hit
> that case often?

It's unlikely, I suppose. I'll try removing it.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +1704,2 @@
> >        int32_t attrNameSpace = kNameSpaceID_None;
> > +      if (ns) {
> 
> nit: you can do
> > if (RefPtr<nsAtom> ns = attr->mNamespaceURL) {
> >   ...
> > }

Fixed.

> 
> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +1386,5 @@
> >        case eStyleContentType_Attr: {
> > +        // XXXbholley: We don't correctly serialize namespaces here. Doing so
> > +        // would require either storing the prefix on the nsStyleContentAttr,
> > +        // or poking at the namespaces in the stylesheet to map from the
> > +        // namespace URL.
> 
> I guess it isn't a big problem to just store prefix in nsStyleContentAttr.
> Is there any concern for doing that?

I played around with it a bit, but couldn't figure out how to write a testcase to exercise it (suggestions?). In any case, this was already broken and not very important, so I'm just going to punt on it for now.

> 
> ::: layout/style/nsStyleStruct.cpp
> @@ +4143,5 @@
> >    } else if (mType == eStyleContentType_Counter ||
> >               mType == eStyleContentType_Counters) {
> >      mContent.mCounters->Release();
> > +  } else if (mType == eStyleContentType_String) {
> > +    MOZ_ASSERT(mContent.mString);
> 
> This assertion doesn't seem to be very useful since `free` is a no-op if the
> pointer is nullptr. Actually, there is no guarantee from the previous code
> that mContent.mString must have value when mType is eStyleContentType_String.

Ok, I'll remove the assertion.
> 
> @@ +4151,1 @@
> >    }
> 
> Please instead assert here that mContent.mString is null to ensure that we
> are not leaking anything.

I assume you mean in a new |else| branch? I can add that.

> ::: servo/components/style/properties/gecko.mako.rs
> @@ +5515,1 @@
> >                                      },
> 
> Hmmm... I really hope we have some sugar for refptr and nsAtom... but I
> guess this is fine for now.

Yeah :\

> 
> @@ +5612,5 @@
> > +                            let ns = if s.mNamespaceURL.mRawPtr.is_null() {
> > +                                None
> > +                            } else {
> > +                                // FIXME(bholley): We don't have any way to get the prefix here. :-(
> > +                                let prefix = Atom::from("");
> 
> `atom!("")`.

Fixed.

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add a ToLowerCaseASCII utility method for nsAtom. v4

Removed duplicate (and unused) macro which caused redefinition failures.

Comment 18

[Bobby Holley (:bholley)]

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

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 8965482 [details] [diff] [review]
Part 3 - Add a ToLowerCaseASCII utility method for nsAtom. v4

Review of attachment 8965482 [details] [diff] [review]:
-----------------------------------------------------------------

r+ provided that the comments are addressed.

::: intl/unicharutil/util/nsUnicharUtils.cpp
@@ +192,5 @@
> +void
> +ToLowerCaseASCII(const char16_t *aIn, char16_t *aOut, uint32_t aLen)
> +{
> +  for (uint32_t i = 0; i < aLen; i++) {
> +    char32_t ch = aIn[i];

This should be char16_t.

@@ +193,5 @@
> +ToLowerCaseASCII(const char16_t *aIn, char16_t *aOut, uint32_t aLen)
> +{
> +  for (uint32_t i = 0; i < aLen; i++) {
> +    char32_t ch = aIn[i];
> +    aOut[i] = IS_ASCII_UPPER(ch) ? ToLowerCaseASCII(ch) : ch;

Please make this line
aOut[i] = IS_ASCII_UPPER(ch) ? (ch + 0x20) : ch;

(ToLowerCaseASCII() assumes that the compiler isn't smart enough to optimize IS_ASCII_UPPER into a single branch, and optimizing it to a single branch is simple, so I'd be surprised if our compilers didn't do it.)

::: intl/unicharutil/util/nsUnicharUtils.h
@@ +33,4 @@
>  void ToUpperCase(const nsAString& aSource, nsAString& aDest);
>  
>  uint32_t ToLowerCase(uint32_t aChar);
> +uint32_t ToLowerCaseASCII(uint32_t aChar);

Then adding this isn't necessary.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8965084 [details] [diff] [review]
Part 5 - Stop using Gecko namespace ids in servo. v1

Review of attachment 8965084 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the drive-by nit-reporting pass :)

::: servo/components/style/gecko/selector_parser.rs
@@ +469,5 @@
>          ))
>      }
>  
>      fn default_namespace(&self) -> Option<Namespace> {
> +        self.namespaces.default.clone().as_ref().map(|ns| ns.clone())

Why the double clone? Isn't self.namespaces.default.clone() enough (It's an Option<Namespace>?

@@ +474,4 @@
>      }
>  
>      fn namespace_for_prefix(&self, prefix: &Atom) -> Option<Namespace> {
> +        self.namespaces.prefixes.get(prefix).map(|ns| ns.clone())

nit: cloned() instead of map(|ns| ns.clone()) is a bit nicer.

Comment 21

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/20577

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2189673fb8ea
Add test for case insensitivity for content: attr() identifiers in html documents. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/447ff1af0d59
Add an atom overload for namespace registration. r=bz
https://hg.mozilla.org/integration/autoland/rev/56564b7c2fe7
Add a ToLowerCaseASCII utility method for nsAtom. r=hisvonen
https://hg.mozilla.org/integration/autoland/rev/9b93666a3de9
Stop using Gecko namespace ids in servo. r=xidorn

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2189673fb8ea
https://hg.mozilla.org/mozilla-central/rev/447ff1af0d59
https://hg.mozilla.org/mozilla-central/rev/56564b7c2fe7
https://hg.mozilla.org/mozilla-central/rev/9b93666a3de9

Comment 24

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10367 for changes under testing/web-platform/tests