Remove IPDL state machine support

RESOLVED FIXED in Firefox 53

Status

()

Core
IPC
RESOLVED FIXED
2 years ago
28 days ago

People

(Reporter: billm, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

State machine checking doesn't work and no one uses it. We're rewriting the IPDL code, so I think we should just drop this. If we want to add it back in he future, the code will probably look pretty different anyway.
(Assignee)

Comment 1

2 years ago
I'll take a look at this.
Assignee: nobody → continuation
(Assignee)

Comment 2

2 years ago
One question I have is, what to do with the various tests that look like they are only testing state machine things? The easiest thing to do is just delete the state machine syntax.
We should just remove those tests.
Having the state machine was a interesting design of IPDL but in reality it's very hard to make the state machine work. We have too many protocols that just doesn't fit into the model. I support dropping the state machine but we probably want to keep the initial and dead state.
(Assignee)

Updated

2 years ago
Depends on: 1317180
(Assignee)

Updated

2 years ago
Depends on: 1319595
(Assignee)

Comment 5

2 years ago
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4)
> I support dropping the state machine
> but we probably want to keep the initial and dead state.

What would that get us? (I don't know much about how this works.)

For reference, I only see two protocols outside of the test directory that use state machine stuff at all, PWebBrowserPersistDocument and PPluginBackgroundDestroyer. The latter is quite trivial.
(In reply to Andrew McCreight [:mccr8] from comment #5)
> (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4)
> > I support dropping the state machine
> > but we probably want to keep the initial and dead state.
> 
> What would that get us? (I don't know much about how this works.)

We want to make sure that someone can't send messages to an actor that has been __delete__d but whose object is still alive (or one where the constructor message hasn't been sent). We keep track of that using the mState and Transition code that's generated by default for each actor. We could just have a more specialized field to keep track of whether an actor is alive or dead.
(Assignee)

Comment 7

2 years ago
(In reply to Bill McCloskey (:billm) from comment #6)
> We want to make sure that someone can't send messages to an actor that has
> been __delete__d but whose object is still alive (or one where the
> constructor message hasn't been sent).

Oh, I see. I think for a first pass I'll just ensure we generate the same code with my deletion patches. Hopefully I can delete a bunch of code even with that restriction.
(Assignee)

Comment 8

2 years ago
Created attachment 8814989 [details] [diff] [review]
Remove state machine stuff from frontend. WIP.

Here's what I think we can remove. The resulting code still has all of the default state checking things that lower.py creates. I checked that it does not change the output for a basic IPDL file (PColorPicker). It should remove any mention of state related things from parser.py, type.py and ast.py. This does not include any changes to IPDL files to remove their usage of state machines.

MozReview-Commit-ID: 1fABHR3zJx
(Assignee)

Comment 9

2 years ago
All told, it looks like it removes around 700 lines of Python code.
(Assignee)

Updated

2 years ago
Depends on: 1320755
(Assignee)

Updated

2 years ago
Blocks: 1321029
(Assignee)

Comment 10

2 years ago
There's a bunch of patches here, but aside from the last one they are all only deleting state machine stuff from IPDL files. Bill is on vacation, and there's the all hands next week, but I'll just put these up for review for whenever he has some time again. No hurry.
(Assignee)

Comment 12

2 years ago
Well, he's not accepting review requests, so I'll just wait.
(Assignee)

Comment 13

2 years ago
Bill, do you want to review this, or should I ask, say, Kan-ru? Thanks.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 14

2 years ago
In person Bill said he could review it.
Flags: needinfo?(wmccloskey)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

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

Comment 22

2 years ago
mozreview-review
Comment on attachment 8817433 [details]
Bug 1316757, part 1 - Remove state machine user syntax from non-test IPDL files.

https://reviewboard.mozilla.org/r/97020/#review98438
Attachment #8817433 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8817434 [details]
Bug 1316757, part 2 - Remove assertions involving state() from cxx/ unit tests.

https://reviewboard.mozilla.org/r/97022/#review98440

::: ipc/ipdl/test/cxx/TestHangs.cpp
(Diff revision 1)
>  TestHangsParent::AnswerStackFrame()
>  {
>      ++mNumAnswerStackFrame;
>  
> -    // XXX This assertion will get deleted as part of bug 1316757.
> -    MOZ_ASSERT((PTestHangs::HANG != state()) == (mNumAnswerStackFrame == 1));

Could you comment out these assertions rather than deleting them? Ideally we would manually add state vars to these protocols to track the same thing, but I found it too confusing to follow the logic myself.  So let's just leave them here commented out in case anyone is curious.
Attachment #8817434 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8817435 [details]
Bug 1316757, part 3 - Delete IPDL unit tests that look like they only test state machine stuff.

https://reviewboard.mozilla.org/r/97024/#review98442
Attachment #8817435 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8817436 [details]
Bug 1316757, part 4 - Remove trivial state machines from IPDL unit tests.

https://reviewboard.mozilla.org/r/97026/#review98444
Attachment #8817436 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 26

2 years ago
mozreview-review
Comment on attachment 8817437 [details]
Bug 1316757, part 5 - Comment out simple uses of state machine from cxx/ unit tests.

https://reviewboard.mozilla.org/r/97028/#review98446

Can you comment these out instead of deleting them?
Attachment #8817437 - Flags: review?(wmccloskey)
(Reporter)

Comment 27

2 years ago
mozreview-review
Comment on attachment 8817438 [details]
Bug 1316757, part 6 - Comment out various race-related state machines.

https://reviewboard.mozilla.org/r/97030/#review98448
Attachment #8817438 - Flags: review?(wmccloskey) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 years ago
(In reply to Bill McCloskey (:billm) from comment #23)
> So let's just leave them here commented out in case anyone is curious.

Sure. I've fixed that, and changed the other patch to also just comment them out.
(Reporter)

Comment 36

2 years ago
mozreview-review
Comment on attachment 8817437 [details]
Bug 1316757, part 5 - Comment out simple uses of state machine from cxx/ unit tests.

https://reviewboard.mozilla.org/r/97028/#review98740
Attachment #8817437 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 37

2 years ago
mozreview-review
Comment on attachment 8817439 [details]
Bug 1316757, part 7 - Remove IPDL state machine from frontend.

https://reviewboard.mozilla.org/r/97032/#review98742
Attachment #8817439 - Flags: review?(wmccloskey) → review+

Comment 38

2 years ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a19a666b1e24
part 1 - Remove state machine user syntax from non-test IPDL files. r=billm
https://hg.mozilla.org/integration/autoland/rev/1ca9a1f12865
part 2 - Remove assertions involving state() from cxx/ unit tests. r=billm
https://hg.mozilla.org/integration/autoland/rev/5f387c5e2c70
part 3 - Delete IPDL unit tests that look like they only test state machine stuff. r=billm
https://hg.mozilla.org/integration/autoland/rev/5c9304341b88
part 4 - Remove trivial state machines from IPDL unit tests. r=billm
https://hg.mozilla.org/integration/autoland/rev/bc740487187b
part 5 - Comment out simple uses of state machine from cxx/ unit tests. r=billm
https://hg.mozilla.org/integration/autoland/rev/e3d3cea91f08
part 6 - Comment out various race-related state machines. r=billm
https://hg.mozilla.org/integration/autoland/rev/45b48d130578
part 7 - Remove IPDL state machine from frontend. r=billm
See Also: → bug 1457536
You need to log in before you can comment on or make changes to this bug.