1826705 - Make destroy method of MessageHandler modules optional

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P3)

Description

[Julian Descottes [:jdescottes]]

The windowglobal input module landed without implementing a destroy method, which means that we fallback to the base Module destroy method, which throws at the moment: https://searchfox.org/mozilla-central/rev/9e3413f54ed6c9165b5659558091d766d34a731f/remote/shared/messagehandler/Module.sys.mjs#35-37

  /**
   * Clean-up the module instance.
   *
   * It's required to be implemented in the sub class.
   */
  destroy() {
    throw new Error("Not implemented");
  }

The idea behind throwing was to encourage module implementations to properly handle cleanup on destroy. However we don't have a good mechanism to detect missing destroy methods, so at the moment forgetting a destroy mostly leads to error spam (and potentially some hard to predict side effects, since it will prevent the rest of the MessageHandler to be destroyed).

Looking at our 13 module classes in bidi, 6 implement a meaningful destroy, 5 just have an empty function (4 actually, 5 when including the one missing from the input module).

We should improve that, either by making destroy really mandatory (explicit assertion?) or by making the base class destroy not throwing.

Comment 1

[Anwar Sadat Ayub]

Hi. I want to take a look at this bug. I will appreciate any guide

Comment 2

[Julian Descottes [:jdescottes]]

(In reply to Anwar Sadat Ayub from comment #1)

Hi. I want to take a look at this bug. I will appreciate any guide

Hi, thanks for offering to work on this! You can look at the contributor documentation over at https://firefox-source-docs.mozilla.org/ to get started, and I will assign the bug to you for now.

We are still discussing a bit what we should actually do here, but most likely we will change the base Module destroy defined at https://searchfox.org/mozilla-central/rev/a4fb94aab371d8650d194558c77879f5b8842380/remote/shared/messagehandler/Module.sys.mjs#36 to either log something or simply do nothing.

I will update this bug once we reached a decision.

Comment 3

[Julian Descottes [:jdescottes]]

So after discussing we decided to replace the throw new Error("Not implemented"); in the destroy method by a simple warning, using logger.warn.

This means you will need to import and create a "logger" here, since we don't have any in the Module class yet.
You can look at other classes which use a logger to get an idea of what needs to be done, eg https://searchfox.org/mozilla-central/rev/a4fb94aab371d8650d194558c77879f5b8842380/remote/shared/messagehandler/sessiondata/SessionData.sys.mjs

As for the warning message itself, it should say something such as "Module <module name> is missing a destroy method" or something like this.

Comment 4

[Anwar Sadat Ayub]

As part of the warning message, do I replace the <module name> withe the private property #messageHandler?

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Anwar Sadat Ayub from comment #4)

As part of the warning message, do I replace the <module name> withe the private property #messageHandler?

You should rather use ${this.constructor.name} which should give us something like BrowsingContextModule for the browsingcontext module etc... Ideally it would be great to have the module name and its layer (root/windowglobal/...), but that's harder to implement, we don't really have the info available at the moment.

Comment 6

[Anwar Sadat Ayub]

Thanks (In reply to Julian Descottes [:jdescottes] from comment #5)

(In reply to Anwar Sadat Ayub from comment #4)

As part of the warning message, do I replace the <module name> withe the private property #messageHandler?

You should rather use ${this.constructor.name} which should give us something like BrowsingContextModule for the browsingcontext module etc... Ideally it would be great to have the module name and its layer (root/windowglobal/...), but that's harder to implement, we don't really have the info available at the moment.

Thanks

Comment 7

[Anwar Sadat Ayub]

Attachment: Bug 1826705 - Make destroy method of MessageHandler modules optional. r=jdescottes

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cea73e20885
Make destroy method of MessageHandler modules optional. r=jdescottes,webdriver-reviewers

Comment 9

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/1cea73e20885