Closed Bug 1396686 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: zombie, Assigned: zombie)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1396686 - Provide info which onMessage listener's response handle went out of scope
59 bytes, text/x-review-board-request
kmag
: review+
Details 
      No description provided.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Attachment #8904388 - Flags: review?(kmaglione+bmo)
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.
Attachment #8904388 - Flags: review?(kmaglione+bmo)
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 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 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.
Attachment #8904388 - Flags: review?(kmaglione+bmo) → review+
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2b6ef963b47b
Provide info which onMessage listener's response handle went out of scope r=kmag
https://hg.mozilla.org/mozilla-central/rev/2b6ef963b47b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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!
Flags: needinfo?(tomica)
Not needed here, thanks.
Flags: needinfo?(tomica) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: