Closed Bug 1315391 Opened 3 years ago Closed 3 years ago

Rename all `disconnect` methods to `destroy` in actors

Categories

(DevTools :: General, defect, P1)

49 Branch
defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: jryans, Assigned: jryans)

Details

Attachments

(2 files)

Some actors (those whose parents don't use protocol.js) use a method named `disconnect` to implement cleanup, while children on protocol.js actors use `destroy`.

It's very confusing to have two names for one concept when writing actors.  Let's rename everything to `destroy`.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment on attachment 8810557 [details]
Bug 1315391 - Rename all `disconnect` methods to `destroy` in actors.

https://reviewboard.mozilla.org/r/92824/#review93146

Looks good, I imagine tests are going to be better reviewers than me on such patch!
Attachment #8810557 - Flags: review?(poirot.alex) → review+
Comment on attachment 8810558 [details]
Bug 1315391 - Clean up actor destruction after changing to `destroy`.

https://reviewboard.mozilla.org/r/92826/#review93142

::: devtools/shared/protocol.js:807
(Diff revision 1)
> -    let parent = this.parent();
> +    let parent;
> +    try {
> +      parent = this.parent();
> +    } catch (e) {
> +      console.error(`${this}'s parent() doesn't work during destroy()!`);
> +    }

This is mysterious. Do you have more info about this?
Does that actually happen?
Isn't that about calling destroy() multiple times?
It looks like it would throw if the actor has been already removed from the pools.
Comment on attachment 8810558 [details]
Bug 1315391 - Clean up actor destruction after changing to `destroy`.

https://reviewboard.mozilla.org/r/92826/#review93142

> This is mysterious. Do you have more info about this?
> Does that actually happen?
> Isn't that about calling destroy() multiple times?
> It looks like it would throw if the actor has been already removed from the pools.

Some actors (fixed in this patch) would store some object (their parent actor) on `this.parent`, replacing the `parent()` method from the actor's own prototype from the internals of protocol.js.  (Perhaps protocol.js should use a different name, so that people writing actors can safely use the property `parent` for their own needs?)

The problems I am trying to catch here are:

* `this.parent` is no longer a method so it throws when called
* `this.parent()` depends on some additional property already cleared before `destroy()` (this was storage while I working on this patch, which depends on `this.storageActor` to be set to evaluate `get conn()` which is called inside `this.parent()`)
Comment on attachment 8810558 [details]
Bug 1315391 - Clean up actor destruction after changing to `destroy`.

https://reviewboard.mozilla.org/r/92826/#review93244

::: devtools/shared/protocol.js:807
(Diff revision 1)
> -    let parent = this.parent();
> +    let parent;
> +    try {
> +      parent = this.parent();
> +    } catch (e) {
> +      console.error(`${this}'s parent() doesn't work during destroy()!`);
> +    }

Ok. So in theory it isn't needed thanks to all the other fixes?
I'm just wondering if we are going to hide breakages that can be highlighted with an exception reported while running tests?
Shouldn't we keep throwing?

The "parent" attribute being overloaded is pretty bad scenario. But I don't see what we could do except freezing the prototype...
Attachment #8810558 - Flags: review?(poirot.alex) → review+
Comment on attachment 8810558 [details]
Bug 1315391 - Clean up actor destruction after changing to `destroy`.

https://reviewboard.mozilla.org/r/92826/#review93244

> Ok. So in theory it isn't needed thanks to all the other fixes?
> I'm just wondering if we are going to hide breakages that can be highlighted with an exception reported while running tests?
> Shouldn't we keep throwing?
> 
> The "parent" attribute being overloaded is pretty bad scenario. But I don't see what we could do except freezing the prototype...

Okay, I'll just revert this log then...  Hopefully it will be more obvious next time around.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/44d12580b8c6
Rename all `disconnect` methods to `destroy` in actors. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/004c203d5917
Clean up actor destruction after changing to `destroy`. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/44d12580b8c6
https://hg.mozilla.org/mozilla-central/rev/004c203d5917
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.