Closed
Bug 1316757
Opened 8 years ago
Closed 7 years ago
Remove IPDL state machine support
Categories
(Core :: IPC, defect)
Core
IPC
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)
58 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
billm
:
review+
|
Details |
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 2•8 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.
Reporter | ||
Comment 3•8 years ago
|
||
We should just remove those tests.
Comment 4•8 years ago
|
||
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 | ||
Comment 5•7 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.
Reporter | ||
Comment 6•7 years ago
|
||
(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•7 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•7 years ago
|
||
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•7 years ago
|
||
All told, it looks like it removes around 700 lines of Python code.
Assignee | ||
Comment 10•7 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 11•7 years ago
|
||
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18ea4d45ced18deffd92dfbcbc23b955f8ef730b
Assignee | ||
Comment 12•7 years ago
|
||
Well, he's not accepting review requests, so I'll just wait.
Assignee | ||
Comment 13•7 years ago
|
||
Bill, do you want to review this, or should I ask, say, Kan-ru? Thanks.
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 14•7 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•7 years ago
|
Attachment #8814989 -
Attachment is obsolete: true
Reporter | ||
Comment 22•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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
Comment 39•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a19a666b1e24 https://hg.mozilla.org/mozilla-central/rev/1ca9a1f12865 https://hg.mozilla.org/mozilla-central/rev/5f387c5e2c70 https://hg.mozilla.org/mozilla-central/rev/5c9304341b88 https://hg.mozilla.org/mozilla-central/rev/bc740487187b https://hg.mozilla.org/mozilla-central/rev/e3d3cea91f08 https://hg.mozilla.org/mozilla-central/rev/45b48d130578
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•