1015278 - When sharing the framemetrics cross-process, the content description std::string causes crashes

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Crash backtrace

I enabled progressive painting on b2g yesterday but it was backed out because of crashes on the R-oop b2g desktop test. After much futzing around I was able to reproduce this locally, and it appears to be a result of the std::string mContentDescription that is in the FrameMetrics object. I suspect that because std::string uses heap memory to store data it cannot be copied into the shmem without special consideration. If I remove mContentDescription from the FrameMetrics the crash goes away. I'm attaching the backtrace taken from the tbpl log.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I'm leaning towards replacing the std::string with a fixed-length char[] as the best option right now. It means content descriptions will be truncated, but generally the first 20 chars or so should be sufficient for APZ tree-dumping needs. Thoughts?

Comment 2

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> I'm leaning towards replacing the std::string with a fixed-length char[] as
> the best option right now. It means content descriptions will be truncated,
> but generally the first 20 chars or so should be sufficient for APZ
> tree-dumping needs. Thoughts?

This would have the effect of increasing the size of the FrameMetrics object by 16 bytes (20 - sizeof(std::string)). One of the nice things about using std::string for mContentDescription is that if the "apz.printtree" is off, no one sets the string and it just acts as a null pointer without taking up extra memory.

I wonder if emptying the string (for example by with 'mContentDescription.swap(std::string())') before copying it to the shared memory would fix the crash? The reader of the shared frame metrics wouldn't get to see the content description, but I don't think it needs to to begin with.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #2)
> This would have the effect of increasing the size of the FrameMetrics object
> by 16 bytes (20 - sizeof(std::string)).

On my OS X machine sizeof(std::string) is 24 bytes. So this change actually reduces the size of FrameMetrics.

> I wonder if emptying the string (for example by with
> 'mContentDescription.swap(std::string())') before copying it to the shared
> memory would fix the crash? The reader of the shared frame metrics wouldn't
> get to see the content description, but I don't think it needs to to begin
> with.

I was running the test with apz.printtree disabled, and it was still crashing. I assume that under these conditions the mContentDescription would already be empty, and so emptying it out again shouldn't make a difference.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This is what I have so far. Fixes the crash locally, but I'll do a try push to confirm.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

https://tbpl.mozilla.org/?tree=Try&rev=c8f043643d01

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> (In reply to Botond Ballo [:botond] from comment #2)
> > This would have the effect of increasing the size of the FrameMetrics object
> > by 16 bytes (20 - sizeof(std::string)).
> 
> On my OS X machine sizeof(std::string) is 24 bytes. So this change actually
> reduces the size of FrameMetrics.

Huh. I guess some std::string implementations are better than others.

On Linux with gcc 4.7, sizeof(std::string) is the size of a pointer, so 8 on 64-bit machines and 4 on 32-bit machines.

Of course, what matters is what sizeof(std::string) is on B2G, and I just checked and it's 24 :( - so please ignore my argument above :)

> > I wonder if emptying the string (for example by with
> > 'mContentDescription.swap(std::string())') before copying it to the shared
> > memory would fix the crash? The reader of the shared frame metrics wouldn't
> > get to see the content description, but I don't think it needs to to begin
> > with.
> 
> I was running the test with apz.printtree disabled, and it was still
> crashing. I assume that under these conditions the mContentDescription would
> already be empty, and so emptying it out again shouldn't make a difference.

Right. My notion that an empty string could be copied safely into shared memory assumed the sort of representation for std::strings that has sizeof(std::string) = size of pointer.

We could still get to a pointer-size-only representation by making it a plain char* and managing any dynamic memory ourselves, but that's probably not worth the effort - so I think the char[20] is fine.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8427878 [details] [diff] [review]
Patch

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

Drive-by review.

Please add a comment above the declaration of FrameMetrics saying that this class is stored in shared memory and thus must be a "Plain Old Data (POD)" type, with no members that use dynamic memory and usch.

::: gfx/layers/FrameMetrics.h
@@ +389,5 @@
>    void SetContentDescription(const std::string& aContentDescription)
>    {
> +    strncpy(mContentDescription, aContentDescription.c_str(), sizeof(mContentDescription));
> +    // forcibly null-terminate in case aContentDescription is too long
> +    mContentDescription[sizeof(mContentDescription) - 1] = 0;

s/0/'\0'

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Will do. Thanks for the review :)

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8427878 [details] [diff] [review]
Patch

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

::: gfx/layers/FrameMetrics.h
@@ +389,5 @@
>    void SetContentDescription(const std::string& aContentDescription)
>    {
> +    strncpy(mContentDescription, aContentDescription.c_str(), sizeof(mContentDescription));
> +    // forcibly null-terminate in case aContentDescription is too long
> +    mContentDescription[sizeof(mContentDescription) - 1] = 0;

Also, it's better to write 'sizeof(mContentDescription)/sizeof(mContentDescription[0])' here so that it remains correct even if someone changes the 'char' to 'char16_t' or something.

Alternatively, you could pull out the 20 into a constant and just repeat the constant.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

v2 - addresses review comments, and uses a different initializer for the char[] so that compilers will (hopefully) complain less and compile more.

https://tbpl.mozilla.org/?tree=Try&rev=fd70b91edae7

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8427928 [details] [diff] [review]
Patch v2

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

::: gfx/layers/FrameMetrics.h
@@ +6,5 @@
>  #ifndef GFX_FRAMEMETRICS_H
>  #define GFX_FRAMEMETRICS_H
>  
>  #include <stdint.h>                     // for uint32_t, uint64_t
> +#include <string>                       // for std::string, strncpy

Technically, the header that provides 'strncpy' is <string.h>, a distinct header from <string>.

@@ +385,5 @@
>    {
>      return mScrollGeneration;
>    }
>  
> +  const std::string GetContentDescription() const

There's no point in putting 'const' on a type returned by value. (In fact, in can inhibit certain optimizations.)

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8427928 [details] [diff] [review]
Patch v2

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

::: gfx/layers/FrameMetrics.h
@@ +92,5 @@
>      , mDisplayPortMargins(0, 0, 0, 0)
>      , mUseDisplayPortMargins(false)
>      , mPresShellId(-1)
> +  {
> +    mContentDescription[0] = '\0';

I read up a bit on array initialization. It turns out you can put the array in the initializer list with no arguments, as in:

  , mPreviousMember(...)
  , mContentDescription()
  , mNextMember(...)

and all elements will be initialized to zero.

I think this is better than only initializing the first element to zero, since then the other 19 elements are left uninitialized, and if we then copy the structure to another process we are potentially leaking random memory contents to another process.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Updated the patch with all the above comments, and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d333f05d2d1

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(I also did a build try run to make sure all the compilers were happy with the initializer change - https://tbpl.mozilla.org/?tree=Try&rev=f58e2545dd27)

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d333f05d2d1