1411893 - Introduce nsStaticAtom

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

nsAtom covers both static and dynamic atoms. But there are quite a few places where we deal exclusively with static atoms, and so we can use raw pointers without worrying about refcounting.

This bug will introduce nsStaticAtom, a trivial sub-class of nsAtom that can be used for these cases.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1411893 - Remove some unnecessary (nsAtom***) casts. .

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1411893 - Introduce nsStaticAtom. .

It's a sub-class of nsAtom, useful for cases where you know you are dealing
exclusively with static atoms. The nice thing about it is that you can use
raw nsStaticAtom pointers instead of RefPtr<>. (In fact, the AddRef/Release
implementations ensure that we'll crash if we use RefPtr<nsStaticAtom>.)

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8922252 [details]
Bug 1411893 - Introduce nsStaticAtom. .

https://reviewboard.mozilla.org/r/193288/#review198502


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/style/CounterStyleManager.cpp:584
(Diff revision 1)
>      : CounterStyle(aStyle)
>      , mName(aName)
>    {
>    }
>  
> -  virtual nsAtom* GetStyleName() const final;
> +  virtual nsStaticAtom* GetStyleName() const final;

Warning: 'virtual' is redundant since the function is already declared 'final' [clang-tidy: modernize-use-override]

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8922252 [details]
Bug 1411893 - Introduce nsStaticAtom. .

https://reviewboard.mozilla.org/r/193288/#review198510

Clearing review because I don't know how this can pass tests, but the general idea makes sense to me, thanks a lot for cleaning up this stuff!

::: layout/style/nsCSSParser.cpp:7956
(Diff revision 1)
>      if (ExpectSymbol(',', true)) {
>        if (!ParseCounterStyleNameValue(type) && !ParseSymbols(type)) {
>          break;
>        }
>      } else {
> -      type.SetAtomIdentValue(do_AddRef(nsGkAtoms::decimal));
> +      type.SetAtomIdentValue(

You could add an `nsStaticAtom*` overload of this function if you want, instead of this. Though given there's only one caller it's perhaps not a big deal.

::: servo/components/style/gecko/media_queries.rs:640
(Diff revision 1)
>                          nsMediaExpression_Range::eMax
>                      } else {
>                          nsMediaExpression_Range::eEqual
>                      };
>  
>                      let atom = Atom::from(feature_name);

`feature_name` comes directly from the CSS parser, and there's nothing to guarantee that this is a static atom. So I'd just use `atom.as_ptr() == *f.mName as *mut _` instead.

Are there really no tests that caught this?

::: servo/components/style/gecko_string_cache/mod.rs:181
(Diff revision 1)
>  
> +    /// Returns the atom as a mutable nsStaticAtom pointer.
> +    #[inline]
> +    pub fn as_static_ptr(&self) -> *mut nsStaticAtom {
> +        assert!(self.is_static());
> +        let const_ptr: *const nsAtom = &self.0;

nit: `self.as_ptr() as *mut nsStaticAtom`? But I don't think this is needed given the above.

::: servo/components/style/gecko_string_cache/mod.rs:406
(Diff revision 1)
> +    #[inline]
> +    fn from(ptr: *mut nsStaticAtom) -> Atom {
> +        assert!(!ptr.is_null());
> +        unsafe {
> +            let ret = Atom(WeakAtom::new(ptr as *mut nsAtom));
> +            assert!(!ret.is_static());

Huh, what? Shouldn't you assert that it _is_ static here?

But I think that we shouldn't add this if it's not needed actually (I don't see any use of it in this patch).

::: servo/components/style/gecko_string_cache/mod.rs:407
(Diff revision 1)
> +    fn from(ptr: *mut nsStaticAtom) -> Atom {
> +        assert!(!ptr.is_null());
> +        unsafe {
> +            let ret = Atom(WeakAtom::new(ptr as *mut nsAtom));
> +            assert!(!ret.is_static());
> +            ret

Err, I lie, there's one use in the media queries code I see. The comment about the assertion still applies though.

::: xpcom/ds/nsAtom.h:114
(Diff revision 1)
> +// nsICSSPseudoElement, which are trivial subclasses used to ensure only
> +// certain atoms are passed to certain functions.
> +class nsStaticAtom : public nsAtom
> +{
> +  // This will detect if RefPtr<nsStaticAtom> is used.
> +  MozExternalRefCountType AddRef() { MOZ_CRASH("don't refcnt nsStaticAtom"); }

nit: I think you can use `= delete;` and have a compiler error instead.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8922252 [details]
Bug 1411893 - Introduce nsStaticAtom. .

https://reviewboard.mozilla.org/r/193288/#review198584

Thanks, I think this makes a couple of places clearer.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8922251 [details]
Bug 1411893 - Remove some unnecessary (nsAtom***) casts. .

https://reviewboard.mozilla.org/r/193286/#review198586

Much nicer, thank you.

Comment 7

[Nicholas Nethercote [inactive]]

> Clearing review because I don't know how this can pass tests, but the
> general idea makes sense to me, thanks a lot for cleaning up this stuff!

I haven't run proper tests yet :)  Thanks for spotting the logic errors.

> > +  MozExternalRefCountType AddRef() { MOZ_CRASH("don't refcnt nsStaticAtom"); }
> 
> nit: I think you can use `= delete;` and have a compiler error instead.

Ah yes! I was going to try that but I forgot about it. I will do that.

Comment 8

[Nicholas Nethercote [inactive]]

> Thanks, I think this makes a couple of places clearer.

BTW, we have all these static arrays that contains nsGkAtoms, and they're all stored as nsAtom** instead of nsAtom*. That's how we end up with nsAtom*** in some places. AFAICT the double-pointer is not necessary; I'm going to try to convert them to single-pointers.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > Thanks, I think this makes a couple of places clearer.
> 
> BTW, we have all these static arrays that contains nsGkAtoms, and they're
> all stored as nsAtom** instead of nsAtom*. That's how we end up with
> nsAtom*** in some places. AFAICT the double-pointer is not necessary; I'm
> going to try to convert them to single-pointers.

Oh, it is necessary, because these are static lists but nsGkAtoms aren't defined statically. Bug 1411469 is needed for that to work.

Comment 10

[Nicholas Nethercote [inactive]]

Try push for my updates patches looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b868b65cf62ffb126a3dbb266c4c7d29a99a18a

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8922251 [details]
Bug 1411893 - Remove some unnecessary (nsAtom***) casts. .

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

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8922252 [details]
Bug 1411893 - Introduce nsStaticAtom. .

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

Comment 13

[Code Review Bot [:reviewbot]]

Comment on attachment 8922252 [details]
Bug 1411893 - Introduce nsStaticAtom. .

https://reviewboard.mozilla.org/r/193288/#review198884


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/style/CounterStyleManager.cpp:584
(Diff revision 2)
>      : CounterStyle(aStyle)
>      , mName(aName)
>    {
>    }
>  
> -  virtual nsAtom* GetStyleName() const final;
> +  virtual nsStaticAtom* GetStyleName() const final;

Warning: 'virtual' is redundant since the function is already declared 'final' [clang-tidy: modernize-use-override]

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8922252 [details]
Bug 1411893 - Introduce nsStaticAtom. .

https://reviewboard.mozilla.org/r/193288/#review198900

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afb8a71fc972
Remove some unnecessary (nsAtom***) casts. r=froydnj.
https://hg.mozilla.org/integration/autoland/rev/42eb6d46aa30
Introduce nsStaticAtom. r=emilio,froydnj.

Comment 16

[Nicholas Nethercote [inactive]]

Thank you for doing the autoland of the Gecko-side patches, Emilio.

Comment 17

[Nicholas Nethercote [inactive]]

https://github.com/servo/servo/pull/19035 was the Servo-side PR.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/afb8a71fc972
https://hg.mozilla.org/mozilla-central/rev/42eb6d46aa30