783451 - Protocol passed through arguments should be a friend class

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

In case like

struct SurfaceDescriptorGralloc {
  PGrallocBuffer buffer;
};

union SurfaceDescriptor {
  SurfaceDescriptorGralloc;
};

union SharedImage {
  SurfaceDescriptor;
};

sync protocol PImageContainer
{
  manager PImageBridge;
child:
  async ReturnImage(SharedImage image);
};

The generated code for |ReturnImage| needs to access PGrallocBuffer's mId field which is private, this forces us to mark PImageContainer as managing seemingly unrelated PGrallocBuffer.

Error:

../../ipc/ipdl/_ipdlheaders/mozilla/layers/PGrallocBufferParent.h: In member function 'void mozilla::layers::PImageContainerParent::Write(mozilla::layers::PGrallocBufferParent*, IPC::Message*, bool)':
../../ipc/ipdl/_ipdlheaders/mozilla/layers/PGrallocBufferParent.h:228: error: 'int32 mozilla::layers::PGrallocBufferParent::mId' is private
objdir-gecko/ipc/ipdl/PImageContainerParent.cpp:845: error: within this context
objdir-gecko/ipc/ipdl/PImageContainerParent.cpp: In member function 'bool mozilla::layers::PImageContainerParent::Read(mozilla::layers::PGrallocBufferParent**, const IPC::Message*, void**, bool)':
objdir-gecko/ipc/ipdl/PImageContainerParent.cpp:875: error: 'mozilla::ipc::RPCChannel::RPCListener' is an inaccessible base of 'mozilla::layers::PGrallocBufferParent'

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

To fix this we might need to eitehr

1. Run two pass parsing to know that PGrallocBuffer needs to be a friend of PImageContainer

or

2. Introduce a new syntax that indicates PGrallocBuffer is a friend of PImageContainer like

 protocol PGrallocBuffer {
   friend PImageContainer;
 child:
   Msg();
 };

Comment 2

[Benjamin Smedberg]

What is the manager of PGrallocBuffer in this case? Adding friend marking is definitely incorrect here. Perhaps asking the common manager ancestor for the ID is the correct behavior.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

PGrallocBuffer has many managers because we used to create it from different protocols.

Comment 4

[Benjamin Smedberg]

That doesn't matter. We know at compile time which manager applies in this particular case.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Remove outdated FIXME comment

It turns out the bug has been fixed in bug 825928. See bug 852734 comment 82.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

TODO: Add a testcase for it.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Add test case

I'll file the FIXME bug if this test case make sense.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 761939 [details] [diff] [review]
Add test case

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

Looks good! Though splinter says these files have windows line endings which you should remove before checkin.

Here's one thing that needs fixing:

::: ipc/ipdl/test/cxx/PTestIndirectProtocolParamFirst.ipdl
@@ +1,3 @@
> +include protocol PTestIndirectProtocolParamManage;
> +// FIXME/bug xxxxxx protocol PTestIndirectProtocolParamSecond is
> +// already included in PTestIndirectProtocolParam.ipdlh

Please actually file a bug or remove this comment.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to ben turner [:bent] from comment #8)
> ::: ipc/ipdl/test/cxx/PTestIndirectProtocolParamFirst.ipdl
> @@ +1,3 @@
> > +include protocol PTestIndirectProtocolParamManage;
> > +// FIXME/bug xxxxxx protocol PTestIndirectProtocolParamSecond is
> > +// already included in PTestIndirectProtocolParam.ipdlh
> 
> Please actually file a bug or remove this comment.

Actually there is already one, bug 792908, I'll fill that.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fae2a068802
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c902b055c44

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fae2a068802
https://hg.mozilla.org/mozilla-central/rev/5c902b055c44