Don't allow implicit "async" in IPDL

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 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

3 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

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

Updated

3 years ago
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)
Reporter

Comment 4

3 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

3 years ago
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

3 years ago
These are the changes required for the second IPDL file that the build process complains about.
Reporter

Comment 7

3 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
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

3 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)
Attachment #8711922 - Flags: review?(wmccloskey) → review+
Reporter

Updated

3 years ago
Attachment #8711948 - Attachment is obsolete: true
Reporter

Comment 11

3 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

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

Comment 14

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