Closed Bug 925317 Opened 6 years ago Closed 6 years ago

Check that shmems are tracked when we send them across processes

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: nrc, Assigned: nrc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

I had a bug where we were holding on to a shmem after it had been released by IPDL. We should make these bugs easier to find by asserting that a shmem is still being tracked by its deallocator before sending it across IPC. Also, if possible, when we open the shmem as a surface.
I had assumed we already did that. This certainly has my vote!
(In reply to Benoit Girard (:BenWa) from comment #1)
> I had assumed we already did that. This certainly has my vote!

Hmm, I thought this is what we discussed adding in Toronto last week? We may well check - I didn't see anything obvious, but there are a lot of places to check and the code is kind of complicated.
Well we may have checks for something things but likely not everything. Let's use this bug to audit and add as many runtime checks as possible.
We should probably check when passing from the compositor to the client too, but we do that far less so its not as important.
Attachment #815445 - Flags: review?(benjamin)
Comment on attachment 815445 [details] [diff] [review]
Client side checks

I'm not the right person to review this, although it seems strange that we have to make ShadowLayerForwarder a friend of ipc::Shmem. Is that just for shmem.AssertInvariants, and could we just make that public?
Attachment #815445 - Flags: review?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 815445 [details] [diff] [review]
> Client side checks
> 
> I'm not the right person to review this, although it seems strange that we
> have to make ShadowLayerForwarder a friend of ipc::Shmem. Is that just for
> shmem.AssertInvariants, and could we just make that public?

Also for mSegment. There is a getter for that, but it is IPDL-internal only so I didn't want to expose it.
Comment on attachment 815445 [details] [diff] [review]
Client side checks

If you know someone better to review, then please feel free to pass it on.
Attachment #815445 - Flags: review?(bgirard)
Attachment #815445 - Flags: review?(bent.mozilla)
Comment on attachment 815445 [details] [diff] [review]
Client side checks

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

:D
Attachment #815445 - Flags: review?(bgirard) → review+
Blocks: 925773
bent: any thoughts on this?
Flags: needinfo?(bent.mozilla)
Comment on attachment 815445 [details] [diff] [review]
Client side checks

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

This looks ok to me, with one change:

::: ipc/glue/Shmem.h
@@ +62,5 @@
>  
>  class Shmem MOZ_FINAL
>  {
>    friend struct IPC::ParamTraits<mozilla::ipc::Shmem>;
> +  friend class mozilla::layers::ShadowLayerForwarder;

Making this #ifdef DEBUG seems safer so that nothing else tries to reach into the Shmem internals. You might want to add a comment about needing this for CheckSurfaceDescriptor.
Attachment #815445 - Flags: review?(bent.mozilla) → review+
Flags: needinfo?(bent.mozilla)
https://hg.mozilla.org/mozilla-central/rev/6c85d3c19012
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.