1391353 - Try to avoid keeping message data alive while waiting for responses

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1391353: Try to avoid keeping message data alive while waiting for responses.

Review commit: https://reviewboard.mozilla.org/r/169800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169800/

Comment 2

[Tomislav Jovanovic :zombie]

Comment on attachment 8898406 [details]
Bug 1391353: Try to avoid keeping message data alive while waiting for responses.

https://reviewboard.mozilla.org/r/169800/#review175266

There is only one instance of "waiting" in these methods (the last one), since the others are not async, and those local variables are not captured by any callbacks.  From my understanding, nulling a local reference only has effect if GC happens during the execution of the rest of the method (which is rare, most GC happens from the event loop), and clearing right before the `return` has virtually no effect.

In any case, r=me since none of this hurts (except for the code noise), but please either drop the cases when it doesn't do much, or explain why my reasoning is wrong.

::: toolkit/components/extensions/MessageChannel.jsm:525
(Diff revision 1)
>      let recipient = options.recipient || {};
>      let responseType = options.responseType || this.RESPONSE_SINGLE;
>  
>      let channelId = ExtensionUtils.getUniqueId();
>      let message = {messageName, channelId, sender, recipient, data, responseType};
> +    data = null;

this is still held in `message`

::: toolkit/components/extensions/MessageChannel.jsm:565
(Diff revision 1)
>      try {
>        target.sendAsyncMessage(MESSAGE_MESSAGE, message);
>      } catch (e) {
>        deferred.reject(e);
>      }
> +    message = null;

What does this accomplish right before the `return`?

::: toolkit/components/extensions/MessageChannel.jsm:640
(Diff revision 1)
>            resolve(handler.receiveMessage(data));
>          }).catch(e => {
>            Cu.reportError(e.stack ? `${e}\n${e.stack}` : e.message || e);
>          });
>        });
> +      data = null;

same

::: toolkit/components/extensions/MessageChannel.jsm:656
(Diff revision 1)
>      };
>      deferred.promise = new Promise((resolve, reject) => {
>        deferred.reject = reject;
>  
>        this._callHandlers(handlers, data).then(resolve, reject);
> +      data = null;

This is the only place where clearing actually matters IMHO.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Tomislav Jovanovic :zombie from comment #2)
> There is only one instance of "waiting" in these methods (the last one),
> since the others are not async, and those local variables are not captured
> by any callbacks.  From my understanding, nulling a local reference only has
> effect if GC happens during the execution of the rest of the method (which
> is rare, most GC happens from the event loop), and clearing right before the
> `return` has virtually no effect.

Ideally, that's true, but there are some weird cases where lexical variables can wind up captured by a closure and held alive much longer than expected.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d625240014aae80b860fb0c51f307aec307438d
Bug 1391353: Try to avoid keeping message data alive while waiting for responses. r=zombie

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d625240014a
Try to avoid keeping message data alive while waiting for responses. r=zombie

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4d625240014a