Closed Bug 1389968 Opened 2 years ago Closed 2 years ago

Add finalization witnesses to onMessage response callbacks/promises

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: zombie)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Currently, when an extension message listener fails to respond to a message, or to resolve the promise that its listener returns, the return channel stays open until the context is destroyed.

We should try to handle this better for the case where the response callback or return promise is destroyed without ever being called or resolved, and close the response channels with an appropriate error.
Priority: -- → P2
See Also: → 1391353
Assignee: kmaglione+bmo → tomica
Status: NEW → ASSIGNED
Attachment #8900008 - Flags: review?(kmaglione+bmo)
Comment on attachment 8900008 [details]
Bug 1389968 - Reject sendMessage() promise when response handle gets GCd

https://reviewboard.mozilla.org/r/171340/#review178136

Thanks!

::: toolkit/components/extensions/ExtensionChild.jsm:28
(Diff revision 2)
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "WitnessService",
> +  "@mozilla.org/toolkit/finalizationwitness;1", "nsIFinalizationWitnessService");

Should probably be called something like `finilizationService`.
Attachment #8900008 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8900008 [details]
Bug 1389968 - Reject sendMessage() promise when response handle gets GCd

https://reviewboard.mozilla.org/r/171340/#review178176

lgtm
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c4eec1e81c10
Reject sendMessage() promise when response handle gets GCd r=kmag
https://hg.mozilla.org/mozilla-central/rev/c4eec1e81c10
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1396686
Duplicate of this bug: 1391623
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.