Closed Bug 1240871 Opened 8 years ago Closed 8 years ago

Don't allow implicit "async" in IPDL

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: mccr8, Assigned: billm)

Details

Attachments

(2 files, 1 obsolete file)

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.
Bill, as IPC module owner, what is your preference here?
Flags: needinfo?(wmccloskey)
Assignee: nobody → continuation
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.
Yes, I'd rather it be explicit as well.
Flags: needinfo?(wmccloskey)
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
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.
These are the changes required for the second IPDL file that the build process complains about.
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
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.
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)
Attachment #8711922 - Flags: review?(wmccloskey) → review+
Attached patch ipdl fixesSplinter Review
Attachment #8712398 - Flags: review?(continuation)
Attachment #8711948 - Attachment is obsolete: true
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+
I'll assign this to Bill because he did the hard part.
Assignee: nobody → wmccloskey
https://hg.mozilla.org/mozilla-central/rev/b04361fcbafc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.