Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Depends on 2 bugs)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

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 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 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.
Attachment #8922252 - Flags: review?(emilio)
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.
Attachment #8922252 - Flags: review?(nfroyd) → review+
Comment on attachment 8922251 [details]
Bug 1411893 - Remove some unnecessary (nsAtom***) casts. .

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

Much nicer, thank you.
Attachment #8922251 - Flags: review?(nfroyd) → review+
> 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.
> 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.
(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 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 on attachment 8922252 [details]
Bug 1411893 - Introduce nsStaticAtom. .

https://reviewboard.mozilla.org/r/193288/#review198900
Attachment #8922252 - Flags: review?(emilio) → review+
Pushed by ecoal95@gmail.com:
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.
Thank you for doing the autoland of the Gecko-side patches, Emilio.
Assignee: nobody → n.nethercote
Depends on: 1433675
You need to log in before you can comment on or make changes to this bug.