1396686 - Provide info which onMessage listener's response handle went out of scope

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Comment 1

[Tomislav Jovanovic :zombie]

Attachment: Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

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

Comment 2

[Tomislav Jovanovic :zombie]

Comment on attachment 8904388 [details]
Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176188/diff/1-2/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8904388 [details]
Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

https://reviewboard.mozilla.org/r/176188/#review181132

This is mostly good. I'm just worried about the way we're getting the caller frame.

::: toolkit/components/extensions/ExtensionChild.jsm:103
(Diff revision 2)
>          reject(error);
>        });
>      });
>    },
> -  observe(subject, topic, id) {
> -    MessageChannel.abortChannel(id, {message: "Response handle went out of scope"});
> +  observe(subject, topic, tag) {
> +    const [channelId, fileName] = tag.split("|", 2);

The second argument to `.split()` doesn't do anything useful. It essentially just truncates the result array to that length. Please use '\0' as a separator character.

::: toolkit/components/extensions/ExtensionChild.jsm:104
(Diff revision 2)
>        });
>      });
>    },
> -  observe(subject, topic, id) {
> -    MessageChannel.abortChannel(id, {message: "Response handle went out of scope"});
> +  observe(subject, topic, tag) {
> +    const [channelId, fileName] = tag.split("|", 2);
> +    const message = `Promised response from onMessage listener at ${fileName} went out of scope`;

We should include a line number too.

::: toolkit/components/extensions/ExtensionChild.jsm:386
(Diff revision 2)
>      return this.sendMessage(messageManager, msg, recipient, responseCallback);
>    }
>  
>    _onMessage(name, filter) {
>      return new EventManager(this.context, name, fire => {
> +      const match = new Error().stack.match(/@moz-extension:\/\/[\w-]+\/(.+)\n/);

This is going to be quite expensive. We should do something like save `Components.stack.caller` from the `addListener` method and then use the saved value here.

Comment 4

[Tomislav Jovanovic :zombie]

Comment on attachment 8904388 [details]
Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

https://reviewboard.mozilla.org/r/176188/#review181368

::: toolkit/components/extensions/ExtensionChild.jsm:103
(Diff revision 2)
>          reject(error);
>        });
>      });
>    },
> -  observe(subject, topic, id) {
> -    MessageChannel.abortChannel(id, {message: "Response handle went out of scope"});
> +  observe(subject, topic, tag) {
> +    const [channelId, fileName] = tag.split("|", 2);

Argh, an equivalent function from another language (not admitting which ;)  puts the rest of the string in the second element.

::: toolkit/components/extensions/ExtensionChild.jsm:104
(Diff revision 2)
>        });
>      });
>    },
> -  observe(subject, topic, id) {
> -    MessageChannel.abortChannel(id, {message: "Response handle went out of scope"});
> +  observe(subject, topic, tag) {
> +    const [channelId, fileName] = tag.split("|", 2);
> +    const message = `Promised response from onMessage listener at ${fileName} went out of scope`;

This does include both the line and column number (check the error regex from the test), though I probably could have named that better.

::: toolkit/components/extensions/ExtensionChild.jsm:386
(Diff revision 2)
>      return this.sendMessage(messageManager, msg, recipient, responseCallback);
>    }
>  
>    _onMessage(name, filter) {
>      return new EventManager(this.context, name, fire => {
> +      const match = new Error().stack.match(/@moz-extension:\/\/[\w-]+\/(.+)\n/);

This is executed only once per addListener() call, which doesn't seem like a big deal.

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8904388 [details]
Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176188/diff/2-3/

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8904388 [details]
Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

https://reviewboard.mozilla.org/r/176188/#review181472

::: toolkit/components/extensions/ExtensionChild.jsm:103
(Diff revision 3)
>          reject(error);
>        });
>      });
>    },
> -  observe(subject, topic, id) {
> -    MessageChannel.abortChannel(id, {message: "Response handle went out of scope"});
> +  observe(subject, topic, tag) {
> +    const pos = tag.indexOf("|");

It seems the observer service didn't like `\0` in the data string any more than I did.

::: toolkit/components/extensions/ExtensionChild.jsm:390
(Diff revision 3)
>    }
>  
>    _onMessage(name, filter) {
>      return new EventManager(this.context, name, fire => {
> +      const first = new this.context.cloneScope.Error().stack.split("\n")[0];
> +      const location = first.substr(0, first.lastIndexOf(":"));

I can't find a nice and clean way to pass this info inside `addListener` method to here (without changing lots of code).

And since this code should also run exactly once per `addListener` call, I don't think that matters.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8904388 [details]
Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

https://reviewboard.mozilla.org/r/176188/#review181594

::: toolkit/components/extensions/ExtensionChild.jsm:106
(Diff revision 3)
>    },
> -  observe(subject, topic, id) {
> -    MessageChannel.abortChannel(id, {message: "Response handle went out of scope"});
> +  observe(subject, topic, tag) {
> +    const pos = tag.indexOf("|");
> +    const channel = tag.substr(0, pos);
> +    const location = tag.substr(pos + 1);
> +    const fileName = location.substr(0, location.lastIndexOf(":"));

Here too. Why slice the location? And why do we need to pass the fileName at all? In any case, this string still contains the leading "functionName@", which makes it not a filename.

::: toolkit/components/extensions/ExtensionChild.jsm:389
(Diff revision 3)
>      return this.sendMessage(messageManager, msg, recipient, responseCallback);
>    }
>  
>    _onMessage(name, filter) {
>      return new EventManager(this.context, name, fire => {
> +      const first = new this.context.cloneScope.Error().stack.split("\n")[0];

Should probably add a `, 1` to the `split()` call.

::: toolkit/components/extensions/ExtensionChild.jsm:390
(Diff revision 3)
>    }
>  
>    _onMessage(name, filter) {
>      return new EventManager(this.context, name, fire => {
> +      const first = new this.context.cloneScope.Error().stack.split("\n")[0];
> +      const location = first.substr(0, first.lastIndexOf(":"));

Why are you slicing this to `lastIndexOf(":")`? The more information about the location the better.

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8904388 [details]
Bug 1396686 - Provide info which onMessage listener's response handle went out of scope

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176188/diff/3-4/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b6ef963b47b
Provide info which onMessage listener's response handle went out of scope r=kmag

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2b6ef963b47b

Comment 11

[Madalin Cotetiu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

Comment 12

[Tomislav Jovanovic :zombie]

Not needed here, thanks.