Closed Bug 1015278 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file 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.
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?
Flags: needinfo?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(botond)
(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.
Attached patch Patch (obsolete) — Splinter Review
This is what I have so far. Fixes the crash locally, but I'll do a try push to confirm.
Assignee: nobody → bugmail.mozilla
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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'
Attachment #8427878 - Flags: review+
Will do. Thanks for the review :)
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.
Attached patch Patch v2Splinter Review
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
Attachment #8427878 - Attachment is obsolete: true
Attachment #8427928 - Flags: review?(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.)
Attachment #8427928 - Flags: review?(botond) → review+
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.
Updated the patch with all the above comments, and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d333f05d2d1
(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)
https://hg.mozilla.org/mozilla-central/rev/2d333f05d2d1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1023491
Depends on: 1023557
No longer depends on: 1015222
You need to log in before you can comment on or make changes to this bug.