Don't allow implicit "async" in IPDL

RESOLVED FIXED in Firefox 47

Status

()

Core
IPC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: billm)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
A method can be annotated as "sync", "async", or not annotated. The latter two apparently mean the same thing, which has confused both Mossop and I in the last week. We should either always use it or never use it. Personally I'd be inclined for the latter. There are about 500 uses of it in the tree. The first step would be to remove the existing uses, then as a second step somebody could change the IPDL parser to ban "async", hopefully with a little message about how it is implicit.
(Reporter)

Comment 1

2 years ago
Bill, as IPC module owner, what is your preference here?
Flags: needinfo?(wmccloskey)
(Reporter)

Updated

2 years ago
Assignee: nobody → continuation

Comment 2

2 years ago
Isn't it better to be explicit and self-documenting?  If we are going to make a change here, lets require either sync or async to be specified.  I think this would be better practice as it would force new IPDL writers to consider which they want.
(Assignee)

Comment 3

2 years ago
Yes, I'd rather it be explicit as well.
Flags: needinfo?(wmccloskey)
(Reporter)

Comment 4

2 years ago
I'll try taking a look at this. If somebody else is excited to work on it, feel free to take it.
Summary: Remove uses of "async" from IPDL → Don't allow implicit "async" in IPDL
(Reporter)

Comment 5

2 years ago
Created attachment 8711922 [details] [diff] [review]
Remove default async for IPDL methods.

I think this is enough of a change to remove the default "async" for IPDL methods.

I'm not sure what the best way to actually fix the places that are currently relying on the implicit "async". There are going to be a lot of them. With some quick searching, it seems like that just for __delete__ methods alone, there are around 300 places that need to be fixed.
(Reporter)

Comment 6

2 years ago
Created attachment 8711948 [details] [diff] [review]
Example fix for PServiceWorkerManager.ipdl

These are the changes required for the second IPDL file that the build process complains about.
(Reporter)

Comment 7

2 years ago
I don't think I want to deal with writing a script to change almost every line of every IPDL file right now.
Assignee: continuation → nobody
(Assignee)

Comment 8

2 years ago
There are probably better ways to do this, but here's one:

find $SRCDIR -name '*.ipdl' | xargs perl -p -i -e 's/(^[ ]*)([A-Z_][a-zA-Z0-9_]*\()/$1async $2/'

It doesn't fix the indentation for messages with multiple lines of arguments, but that's probably okay.
(Reporter)

Comment 9

2 years ago
Comment on attachment 8711922 [details] [diff] [review]
Remove default async for IPDL methods.

Bill sounded like he might be willing to write the patch to add async everywhere, so I might as well put this up for review. As I said in person, I'm perfectly happen to review such a patch, I just don't want to actually write it.
Attachment #8711922 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8711922 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8712398 [details] [diff] [review]
ipdl fixes
Attachment #8712398 - Flags: review?(continuation)
(Reporter)

Updated

2 years ago
Attachment #8711948 - Attachment is obsolete: true
(Reporter)

Comment 11

2 years ago
Comment on attachment 8712398 [details] [diff] [review]
ipdl fixes

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

Thanks for taking the time to fix up the indentation!

::: ipc/ipdl/test/ipdl/error/race_ViolateSameDirection.ipdl
@@ +1,5 @@
>  protocol race_ViolateSameDirection {
>  child:
> +    async Msg1();
> +    async Msg1_();
> +    async __delete__(); suppressUndeleteableError();

This is missing an async.

::: ipc/ipdl/test/ipdl/error/unreachedDeleteMultiStart.ipdl
@@ +1,3 @@
>  protocol unreachedDeleteMultiStart {
>  child:
> +    async M1(); M2(); __delete__();

Missing 2 async.

::: netwerk/ipc/PNecko.ipdl
@@ +69,1 @@
>                FTPChannelCreationArgs args);

nit: indentation

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +137,1 @@
>                              nsCString clientID);

nit: indentation
Attachment #8712398 - Flags: review?(continuation) → review+
(Reporter)

Comment 12

2 years ago
I'll assign this to Bill because he did the hard part.
Assignee: nobody → wmccloskey

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04361fcbafc

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b04361fcbafc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.