1225682 - disallow member variables of type nsAuto{C,}String

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Nathan Froyd [:froydnj]]

Quoth the XPCOM string guide https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings :

"Another common incorrect pattern is to use nsAutoString/nsAutoCString for member variables. As described in Local Variables, these classes have a built in buffer that make them very large. This means that if you include them in a class, they bloat the class by 64 bytes (nsAutoCString) or 128 bytes (nsAutoString)."

We should write a static analysis to disallow these.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Only allow nsAuto{,C}String on the stack

This obviously breaks the build, and I'll attach one or several patches to remove all consequent static analysis failures once I have green trys.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Hommey [:glandium] from comment #1)
> Created attachment 8688847 [details] [diff] [review]
> Only allow nsAuto{,C}String on the stack
> 
> This obviously breaks the build, and I'll attach one or several patches to
> remove all consequent static analysis failures once I have green trys.

This patch doesn't act identically to the requested analysis, as stack-allocated structs which have nsAuto{,C}Strings in them are still allowed. However, I would think that the thing which we are trying to avoid is the heap allocated ones (as allocating a nsAuto{,C}String in a struct on the stack is mostly equivalent to not-in-a-struct on the stack), so this patch should be good. I just wanted to make sure that the difference was clear :).

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8688847 [details] [diff] [review]
Only allow nsAuto{,C}String on the stack

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

Sure.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in dom/media/

Comment 5

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in dom/

Comment 6

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in js/xpconnect/

Comment 7

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in docshell/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689765 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in docshell/

r=me

Comment 9

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in gfx/

Comment 10

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in image/

Comment 11

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in layout/

Comment 12

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in netwerk/

Comment 13

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in rdf/

Comment 14

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in security/manager/

Comment 15

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in toolkit/

Comment 16

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in uriloader/

Comment 17

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in widget/

Comment 18

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in xpfe/

Comment 19

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in xpcom/

Comment 20

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8689777 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in security/manager/

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

Sounds good.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

I don't want us to prevent nsAuto*String as a member variable. It can be very useful for performance.
Or at least we need to have some way to have pre-allocated strings. Perhaps some other name for such strings?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689763 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in dom/

StringBuilder stuff in nsContentUtils is super performance critical 
(in some silly microbenchmarks).
Because of that, r-

Comment 23

[Mike Hommey [:glandium]]

(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8689763 [details] [diff] [review]
> Don't use nsAuto{,C}String as class member variables in dom/
> 
> StringBuilder stuff in nsContentUtils is super performance critical 
> (in some silly microbenchmarks).
> Because of that, r-

Where is the microbenchmark?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=744830#c51 is one case.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689780 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in uriloader/

This one is weird.  Yes, nsAutoCString is 64 bytes long... but that's exactly the length of an SHA-256 hash.

I guess this is not super-critical, so r=me

Comment 26

[Boris Zbarsky [:bzbarsky]]

The fun part is that StringBuilder stuff all has stack lifetime.  Just not obviously so from inside the class.  So using an autostring there is not unreasonable.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689770 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in layout/

I still need to look at the rest of this, but the use of nsAutoString in the CSS scanner is very purposeful.  We don't want to pay the cost of heap allocations in the common case there.

Comment 28

[Mike Hommey [:glandium]]

(In reply to Boris Zbarsky [:bz] from comment #25)
> Comment on attachment 8689780 [details] [diff] [review]
> Don't use nsAuto{,C}String as class member variables in uriloader/
> 
> This one is weird.  Yes, nsAutoCString is 64 bytes long... but that's
> exactly the length of an SHA-256 hash.

Actually, a SHA-256 is 32 bytes. And since it's fixed size, it could be a char[32] instead of also having all the nsString things like flags, length, and pointer to the actual string...

Comment 29

[Mike Hommey [:glandium]]

But I guess it's a sha-256 in hex form, which makes it indeed a 64 bytes string. The comment still applies, though.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689770 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in layout/

>+++ b/layout/base/nsLayoutUtils.h

So most of the uses of nsSetAttrRunnable are in fact passing in an integer aValue, so we'll get an extra allocation.  And being runnables these are transient....

I guess I can live with this, but doing something with an auto buffer here actually does make sense.

>+++ b/layout/style/nsCSSScanner.h

This change I expect to be bad for performance.  I would rather not do this.  r- for this part.

Comment 31

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8689769 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in image/

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

In both of these cases nsAutoCString was intentionally used to avoid extra dynamic allocations in the extremely common case that the strings involved are small. I think in both of these cases, we can live without that optimization, since ImageURL is quite long-lived and the strings in NewPartResult end up getting assigned elsewhere, presumably sharing the same dynamically allocated buffer. However, I'm not convinced that it's never appropriate to use nsAutoCString as a member variable type, because dynamic allocation has costs and it's frequently the case that strings are small, so I question the premise of this bug.

Comment 32

[Emanuel Hoogeveen [:ehoogeveen]]

I guess the premise is to avoid a footgun, e.g. accidentally bloating a struct with these members. But if it's a conscious decision (e.g. there won't be many of these structs live at any given time, and performance is crucial), perhaps an annotation would be useful (something like MOZ_INPLACE_BUFFER)? (I was going to suggest MOZ_LARGE_MEMBER, but that has unfortunate connotations)

Comment 33

[Mike Hommey [:glandium]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #31)
> Comment on attachment 8689769 [details] [diff] [review]
> Don't use nsAuto{,C}String as class member variables in image/
> 
> Review of attachment 8689769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In both of these cases nsAutoCString was intentionally used to avoid extra
> dynamic allocations in the extremely common case that the strings involved
> are small. I think in both of these cases, we can live without that
> optimization, since ImageURL is quite long-lived and the strings in
> NewPartResult end up getting assigned elsewhere, presumably sharing the same
> dynamically allocated buffer. However, I'm not convinced that it's never
> appropriate to use nsAutoCString as a member variable type, because dynamic
> allocation has costs and it's frequently the case that strings are small, so
> I question the premise of this bug.

Look around for the uses of nsAuto*Strings. There are many cases where it's obvious they were used because of the Auto name, and not because of what they do. It's even more obvious when you see it used for function arguments and *return values*.

Now, there appears to be a few cases where it might actually be desirable, but maybe that's not entirely true either. In the image/ cases, for things like schemes or content types, I don't see why not use something like Atoms (that might be true of the CSSScanner too). In the StringBuilder case, I don't see much reason not to use plain char16_t pointers, although that would require some minor manual handling.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> I don't see why not use something like Atoms (that might be true of the CSSScanner too)

What the CSS scanner uses this string for is an accumulation buffer for the current token.  It needs to be fast, and it needs to be able to change value without thrashing the allocator.  Using an atom (assuming you meant nsIAtom) would not work at all.

Comment 35

[Nathan Froyd [:froydnj]]

Comment on attachment 8689779 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in toolkit/

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

None of these appear to be performance-critical.

Comment 36

[Nathan Froyd [:froydnj]]

Comment on attachment 8689784 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in xpcom/

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

r=me with the changes below.

::: xpcom/reflect/xptinfo/ShimInterfaceInfo.h
@@ +42,5 @@
>      ~ShimInterfaceInfo() {}
>  
>  private:
>      nsIID mIID;
> +    nsCString mName;

We could profitably turn this into a |const char*|, since it only ever refers to names from kComponentsInterfaceShimMap, and those names are statically allocated.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8689768 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in gfx/

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

Sure.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d653011477
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfc8f8660578
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6b93ef7ace
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d6cad904db
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a929584adeb
https://hg.mozilla.org/integration/mozilla-inbound/rev/958002a310a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/210627948092
https://hg.mozilla.org/integration/mozilla-inbound/rev/15bbe7c7bb3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1d53003036
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb12e45c0dd8

Comment 39

[Mike Hommey [:glandium]]

(In reply to Boris Zbarsky [:bz] from comment #25)
> Comment on attachment 8689780 [details] [diff] [review]
> Don't use nsAuto{,C}String as class member variables in uriloader/
> 
> This one is weird.  Yes, nsAutoCString is 64 bytes long... but that's
> exactly the length of an SHA-256 hash.

Actually... not. First, nsIBackgroundFileSaver.idl says that sha256Hash is the SHA-256 hash, in raw bytes. So that's 32 bytes. But it's an ACString, so it still has an additional null terminator, which makes it a 33 bytes string.

But wait for the best part: what BackgroundFileSaver::GetSha256Hash does is assign to the out ACString with operator =, and here is what can be seen after the return from the call to GetSha256Hash from nsExternalAppHandler::OnSaveComplete in gdb:

(gdb) print mHash
$1 = {<nsFixedCString> = {<nsCString> = {<nsACString_internal> = {
        mData = 0x7fffc40481c8 "\177\351\340G*\227\070ɔ\262Y\021\242\365U\370\322\377F\347'tn\346\355\202\354lk\004\210\061", 
        mLength = 32, mFlags = 65541}, <No data fields>}, mFixedCapacity = 63, mFixedBuf = 0x7fffccf70378 ""}, 
  mStorage = "\000", '\345' <repeats 62 times>, <incomplete sequence \345>}

Yup, nsAutoString's inline storage is not even used.

I wouldn't be surprised if in the other cases where "performance matter", the same is also happening (I will check).

Comment 40

[Boris Zbarsky [:bzbarsky]]

Ah, this thing is created as a copy of another string in all cases?  In that case, it only makes sense to make it an autostring if that other string is an autostring using its inline storage.

I'm 99.9% sure this is NOT the scenario in the CSS scanner.  ;)

Comment 41

[Mike Hommey [:glandium]]

(In reply to Boris Zbarsky [:bz] from comment #40)
> I'm 99.9% sure this is NOT the scenario in the CSS scanner.  ;)

Evidence says this is true. However, the story is different for the changes in nsContentUtils.cpp that smaug r-ed.

Comment 42

[Mike Hommey [:glandium]]

Comment on attachment 8689763 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in dom/

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

Evidence shows that on the microbenchmark mentioned in comment 24, of 1,334,797 nsAutoStrings of size <= 63 used for StringBuilder::Unit::mString, 1,274,797  (95.5%) do *NOT* use the inline buffer.

Comment 43

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in xpcom/

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5d653011477
https://hg.mozilla.org/mozilla-central/rev/dfc8f8660578
https://hg.mozilla.org/mozilla-central/rev/2c6b93ef7ace
https://hg.mozilla.org/mozilla-central/rev/45d6cad904db
https://hg.mozilla.org/mozilla-central/rev/9a929584adeb
https://hg.mozilla.org/mozilla-central/rev/958002a310a3
https://hg.mozilla.org/mozilla-central/rev/210627948092
https://hg.mozilla.org/mozilla-central/rev/15bbe7c7bb3c
https://hg.mozilla.org/mozilla-central/rev/fc1d53003036
https://hg.mozilla.org/mozilla-central/rev/bb12e45c0dd8

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8689763 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in dom/

I still don't quite understand why to change StringBuilder, but fine.
(I'll upload a case where the performance does regress because we end up doing more malloc/free)

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Attachment: innerhtml test

When looking at GetAttr of this kind of silly benchmark, that does show quite a bit more allocations. But this is a super artificial test.

Comment 47

[Mike Hommey [:glandium]]

(In reply to Olli Pettay [:smaug] from comment #46)
> Created attachment 8694905 [details]
> innerhtml test
> 
> When looking at GetAttr of this kind of silly benchmark, that does show
> quite a bit more allocations. But this is a super artificial test.

Indeed, this testcase does use the inline storage for all the 3,440,640 uses of nsAutoString from the StringBuilder.

(In reply to Olli Pettay [:smaug] from comment #45)
> Comment on attachment 8689763 [details] [diff] [review]
> Don't use nsAuto{,C}String as class member variables in dom/
> 
> I still don't quite understand why to change StringBuilder.

Because if there are only 2 use cases in the entire tree where using nsAutoString for a member variable /might/ be worth (which in practice is not even a given), we might as well figure how we can do without for those 2. And the thing is, for StringBuilder, we could probably even do without an nsString at all, and that would do even less allocations. I'll try that.

Comment 48

[Mike Hommey [:glandium]]

Attachment: Don't use nsAuto{,C}String as class member variables in dom/

With the code currently in the tree:
https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034
  foo
  div.innerHTML : 1.2ms foo 
  div.outerHTML : 1.2ms <div id="node">foo</div>
  body.innerHTML : 209.4ms
  body.outerHTML : 212ms

https://bugzilla.mozilla.org/attachment.cgi?id=8694905
  Running 20 times Ignoring warm-up run (87) 86 88 86 85 85 86 86 87 86 86 86 86 87 87 113 87 86 87 86 85 avg 87.55 stdev 5.886212704277684 

With the previous patch (using nsString*):
https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034
  foo
  div.innerHTML : 1.2ms foo 
  div.outerHTML : 1.2ms <div id="node">foo</div>
  body.innerHTML : 209ms
  body.outerHTML : 213ms

https://bugzilla.mozilla.org/attachment.cgi?id=8694905
  Running 20 times Ignoring warm-up run (102) 101 104 100 99 101 100 100 99 99 102 100 101 100 100 103 130 100 100 100 99 avg 101.9 stdev 6.5719099202591025 

With this new patch:
https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034
  foo
  div.innerHTML : 1.2ms foo 
  div.outerHTML : 1.2ms <div id="node">foo</div>
  body.innerHTML : 208.4ms
  body.outerHTML : 213.8ms

https://bugzilla.mozilla.org/attachment.cgi?id=8694905
  Running 20 times Ignoring warm-up run (84) 84 86 84 84 84 85 83 84 85 85 84 84 85 86 111 84 84 84 84 86 avg 85.8 stdev 5.8360945845659495 

The number of allocations should be unchanged, or reduced for strings longer than 63 characters, and the size of allocations should be reduced overall.

Comment 49

[Mike Hommey [:glandium]]

Note that considering the noise in the test results when running them multiple times, the only really conclusive thing is that https://bugzilla.mozilla.org/attachment.cgi?id=8694905 *was* slower with the nsString* variant, and that the new patch has the same performance characteristics as the code currently in the tree.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8694983 [details] [diff] [review]
Don't use nsAuto{,C}String as class member variables in dom/

>   void Append(const nsAString& aString)
>   {
>     Unit* u = AddUnit();
>-    u->mString = new nsAutoString(aString);
>+    u->mString = NS_strndup(aString.Data(), aString.Length());
you don't want this, since usually we just share the string data when mString is ns*String

Comment 51

[Mike Hommey [:glandium]]

(In reply to Olli Pettay [:smaug] from comment #50)
> >-    u->mString = new nsAutoString(aString);
> >+    u->mString = NS_strndup(aString.Data(), aString.Length());
> you don't want this, since usually we just share the string data when
> mString is ns*String

mString = new nsAutoString(aString); only shares the string data if aString has the F_SHARED flag, and in that case a nsAutoString gives no advantage, quite the contrary. I don't know if the strings passed in would have that flag. Now, looking at the callers, this code is only used for: DOCUMENT_TYPE_NODE, PROCESSING_INSTRUCTION_NODE, and element nodes that are neither HTML, SVG or MathML. So in practice, this code is never called in neither of the test case you gave me, so I can't tell for sure.

Comment 52

[Mike Hommey [:glandium]]

Some digging tells me those nsStrings are coming from StringBuffer::ToString, which uses shared strings.

Comment 53

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: Don't use nsAuto{,C}String as class member variables in security/manager/ (again)

I accidentally added a new nsAutoString as a member variable in bug 1240118 - this should fix it (luckily it's for a singleton class, but still).

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb38910dcae
(still) don't use nsAutoString as class member variable in security/manager/ r=mgoodwin

Comment 55

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ffb38910dcae

Comment 56

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?

Comment 57

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?

Comment 58

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?

Comment 59

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?

Comment 60

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?

Comment 61

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?

Comment 62

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?

Comment 63

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:glandium, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.