Closed Bug 1316757 Opened 3 years ago Closed 3 years ago

Remove IPDL state machine support

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: billm, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

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.
I'll take a look at this.
Assignee: nobody → continuation
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.
Depends on: 1317180
Depends on: 1319595
(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.
(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.
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
All told, it looks like it removes around 700 lines of Python code.
Depends on: 1320755
Blocks: 1321029
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.
Well, he's not accepting review requests, so I'll just wait.
Bill, do you want to review this, or should I ask, say, Kan-ru? Thanks.
Flags: needinfo?(wmccloskey)
In person Bill said he could review it.
Flags: needinfo?(wmccloskey)
Attachment #8814989 - Attachment is obsolete: true
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+
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+
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+
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+
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)
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+
(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.
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+
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+
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
You need to log in before you can comment on or make changes to this bug.