Closed Bug 1336919 Opened 4 years ago Closed 3 years ago

Disallow new sync IPC message to be created unless absolutely needed

Categories

(Core :: IPC, defect)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: kanru, Assigned: kanru)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

bug 1331674 comment 4

I think we can use a whitelist for all the sync messages and the IPDL compiler will check the list while code gen. New sync messages should not be created unless it's reviewed and added to the whitelist.

The format of the whitelist could be something like:

 {
   "PContent::GetProcessAttributes": {
    "description": "get data for process initialization"
  },
  ...
 }

We could also extend this to js side sendSyncMessage
List of sync messages
Assignee: nobody → kchen
The error message looks like:

 0:05.27 /gecko/dom/ipc/PBrowser.ipdl:207: error: Unknown sync IPC message PBrowser::SyncMessage
 0:05.27 New sync IPC messages must be reviewed and recorded in /gecko/ipc/ipdl/sync-messages.json
 0:05.40 Makefile:27: recipe for target 'ipdl' failed
 0:05.40 make[5]: *** [ipdl] Error 1
Attachment #8837069 - Flags: review?(wmccloskey)
Status: NEW → ASSIGNED
Should we throw an error also when a message is in the list of allowed sync messages but it is async? This way, we can keep the list updated easily (as whenever we convert a sync message to async, we will remove it from the list).
(In reply to Marco Castelluccio [:marco] from comment #4)
> Should we throw an error also when a message is in the list of allowed sync
> messages but it is async? This way, we can keep the list updated easily (as
> whenever we convert a sync message to async, we will remove it from the
> list).

Good idea, I'll add that.
Also check async messages that can be delisted.
Attachment #8837069 - Attachment is obsolete: true
Attachment #8837069 - Flags: review?(wmccloskey)
Attachment #8837082 - Flags: review?(wmccloskey)
Add missing file...
Attachment #8837082 - Attachment is obsolete: true
Attachment #8837082 - Flags: review?(wmccloskey)
Attachment #8837087 - Flags: review?(wmccloskey)
You should probably say they should be reviewed by an IPC peer. Then, some IPC peers should talk to jst about signing up for emails to get changes to that file to make sure nobody lands changes without approval. You should also put a comment about the fact that it needs an IPC peer review in the actual .json file, though I suppose most people won't see it without the error message.
(In reply to Andrew McCreight [:mccr8] from comment #8)
> You should probably say they should be reviewed by an IPC peer. Then, some
> IPC peers should talk to jst about signing up for emails to get changes to
> that file to make sure nobody lands changes without approval. You should
> also put a comment about the fact that it needs an IPC peer review in the
> actual .json file, though I suppose most people won't see it without the
> error message.

I was just thinking about that. Not sure if an IPC peer is a good idea because often it will also need module peers to weigh in.

Sadly the json format does not allow putting comments. I think a comment in the error message should be fine.
(In reply to Andrew McCreight [:mccr8] from comment #8)
> You should probably say they should be reviewed by an IPC peer. Then, some
> IPC peers should talk to jst about signing up for emails to get changes to
> that file to make sure nobody lands changes without approval. You should
> also put a comment about the fact that it needs an IPC peer review in the
> actual .json file, though I suppose most people won't see it without the
> error message.

We can also modify the WebIDL hg hook to reject patches that change sync-messages.json without proper review.

I do actually think that IPC peer review is a good idea here.  I can totally see a lot of module owners may be much more lenient to accepting a new sync message, so I think it would be nice to have a place where IPC peers can say a strong no and see if there is absolutely no other way to solve the problem at hand.  The bar for adding more of these messages must be extremely high (and I personally think it should almost be impossible to add new sync IPC messages going forward.  :-)
Comment on attachment 8837087 [details] [diff] [review]
Add a sync message whitelist and check it in IPDL compile phase

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

If it's not too hard to do in the Makefile, it would be nice to run the C preprocessor on the JSON file so that we can add comments to it. I'd like to see it sorted into sections for things like C++ unit tests, a11y code, CPOWs, plugins, and the rest.

::: ipc/ipdl/ipdl/checker.py
@@ +14,5 @@
> +    def prettyMsgName(self, msg):
> +        return "%s::%s" % (self.currentProtocol, msg)
> +
> +    def errorUnknownSyncMessage(self, loc, msg):
> +        self.errors.append('%s: error: Unknown sync IPC message %s' %

I do think you should ask for review from IPC peers. People sometimes think that they need to make a message sync in order to guarantee that messages are ordered.

@@ +28,5 @@
> +
> +    def visitMessageDecl(self, md):
> +        pn = self.prettyMsgName(md.name)
> +        if md.sendSemantics is not ASYNC and pn not in self.syncMsgList:
> +                self.errorUnknownSyncMessage(md.loc, pn)

This is indented too much.
Attachment #8837087 - Flags: review?(wmccloskey) → review+
(In reply to :Ehsan Akhgari from comment #10)
> (In reply to Andrew McCreight [:mccr8] from comment #8)
> > You should probably say they should be reviewed by an IPC peer. Then, some
> > IPC peers should talk to jst about signing up for emails to get changes to
> > that file to make sure nobody lands changes without approval. You should
> > also put a comment about the fact that it needs an IPC peer review in the
> > actual .json file, though I suppose most people won't see it without the
> > error message.
> 
> We can also modify the WebIDL hg hook to reject patches that change
> sync-messages.json without proper review.

If this isn't too hard, I think it's a good idea.
Depends on: 1340440
json is too restrictive. setting up preprocessor for that is non-trivial. ini file works equally well so I changed to use ini format. If we ever change to use ripdl we can easily convert to toml.
Attachment #8837087 - Attachment is obsolete: true
Attachment #8838435 - Flags: review?(wmccloskey)
Comment on attachment 8838435 [details] [diff] [review]
Add a sync message whitelist and check it in IPDL compile phase v2

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

::: ipc/ipdl/sync-messages.ini
@@ +474,5 @@
> +[PDocAccessible::GetWindowedPluginIAccessible]
> +description =
> +
> +# CPOWs
> +[PBrowser::SyncMessage]

SyncMessage doesn't have anything to do with CPOWs. It should go with "the rest". RpcMessage can stay here I guess.

@@ +966,5 @@
> +[PJavaScript::Has]
> +description =
> +[PJavaScript::HasOwn]
> +description =
> +[PJavaScript::Get]

All the PJavaScript stuff can be moved to the CPOW section.
Attachment #8838435 - Flags: review?(wmccloskey) → review+
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03531a4a9327
Add a sync message whitelist and check it in IPDL compile phase. r=billm
https://hg.mozilla.org/mozilla-central/rev/03531a4a9327
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Thanks again, Kan-Ru!  I think it will be a good idea to notify dev-platform about this too.
(In reply to :Ehsan Akhgari from comment #18)
> Thanks again, Kan-Ru!  I think it will be a good idea to notify dev-platform
> about this too.

Thanks, Ehsan. I've sent an email :)
Depends on: 1345111
Depends on: 1345803
You need to log in before you can comment on or make changes to this bug.