Make sure event handlers and callbacks are not called after a context is destroyed

RESOLVED FIXED in mozilla48

Status

defect
P3
normal
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: kmag, Assigned: kmag, Mentored)

Tracking

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug] triaged)

Attachments

(3 attachments, 2 obsolete attachments)

We don't currently check whether a context is still valid before calling API callbacks or dispatching events under most circumstances. Since nearly all of these are dispatched asynchronously, there are a lot of chances for races where we'll call into a context shortly after it should have been destroyed.

The most obvious places to handle this are in the `wrapPromise` callback handler and the EventManager/SingletonEventManager classes.
Mentor: kmaglione+bmo
Whiteboard: [good first bug]
Besides the context thing, there's also the problem that someone can do:

browser.foo.onbar.addEventListener(listener);
browser.foo.onbar.removeEventListener(listener);
// listener should not be invoked after this point

We don't guarantee that.
I'm just starting out contributing to open source for a class and I'm willing to tackle this bug, I'm trying to learn more about the Mozilla code base.
Hello.

If you haven't built Firefox before, that's the place to start. The simplest way to do that is explained here:

https://developer.mozilla.org/en-US/docs/Artifact_builds

Once you have a working build, you can start making changes to the code. The first two relevant bits of code are here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#235
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#506

It will probably be easiest if you do this in pieces, so I'd suggest starting with wrapPromise. The changes there should be pretty simple. All you have to do is check whether `this.unloaded` is true before calling `runSafe` in the `if (callback)` branch. If it is, you just need to print a warning rather than calling the callbacks.
Posted patch bug1248846.patch (obsolete) — Splinter Review
Attachment #8722251 - Flags: review?(kmaglione+bmo)
Comment on attachment 8722251 [details] [diff] [review]
bug1248846.patch

Review of attachment 8722251 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good, but there are some code style issues (ESLint will reject it on check-in), and I think we need to move a bit of code from Extension.jsm to this class.

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +234,5 @@
>  
>      if (callback) {
>        promise.then(
>          args => {
> +          if(this.context.unloaded){

Space after the `if` and before the `{` please..

Also, there is no `this.context` here. Just `this.unloaded`. And I think we need to move the code that sets `unloaded` from Extension.jsm to the `unload` function in this class.

@@ +238,5 @@
> +          if(this.context.unloaded){
> +            dump("This context is not valid.");
> +          }
> +          else{
> +            if (args instanceof SpreadArgs) {

ESLint will complain about a lonely `if` statement inside an else. Please make this an `else if` of the previous line.

@@ +241,5 @@
> +          else{
> +            if (args instanceof SpreadArgs) {
> +              runSafe(callback, ...args);
> +            } 
> +            else {

Move to the same line as the closing curly brace please. Same for the others.

@@ +248,5 @@
>            }
>          },
>          error => {
> +          if(this.context.unloaded){
> +            dump("Callback received after context was destroyed\n");

I think this should be something like: dump("Callback error received after context was destroyed: ${error.message}\n");

@@ +256,1 @@
>              runSafeSyncWithoutClone(callback);

This needs more indentation.

@@ +516,5 @@
>    },
>  
>    fire(...args) {
>      for (let callback of this.callbacks) {
> +      runSafe(this.context, callback, ...args);   

Please remove the trailing whitespace from this line.
Attachment #8722251 - Flags: review?(kmaglione+bmo)
Attachment #8723370 - Flags: review?(kmaglione+bmo)
Assignee: nobody → tparker12
Priority: -- → P3
Whiteboard: [good first bug] → [good first bug] triaged
Attachment #8722251 - Attachment is obsolete: true
Comment on attachment 8723370 [details] [diff] [review]
bug1248846_revised.patch

Review of attachment 8723370 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like this is a patch on top of your previous patch. Can you submit one patch that contains all of your changes?

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +237,5 @@
>        promise.then(
>          args => {
> +          if (this.unloaded) {
> +            dump("Callback error received after context was destroyed: ${error.message}\n");
> +          } else if {

This is a syntax error. It needs to be `} else if (args instanceof SpreadArgs) {`

@@ +245,1 @@
>              else {

This needs to be moved to the previous line

@@ +245,2 @@
>              else {
>              runSafe(callback, args);

This has too much indentation. It should be indented two spaces from the closing curly brace

@@ +247,3 @@
>            }
> +        }
> +      },

You seem to have removed some indentation from the previous two lines.

@@ +250,5 @@
>          error => {
> +          if(this.unloaded){
> +            dump("Callback error received after context was destroyed: ${error.message}\n");
> +          } 
> +          else {

This needs to be moved to the previous line.

@@ +261,1 @@
>      else {

This line needs to start with a closing curly brace.
Attachment #8723370 - Flags: review?(kmaglione+bmo)
Another attempt at fixing the syntax errors and folded the changes with the last patch as well.
Attachment #8726066 - Flags: review?(kmaglione+bmo)
Comment on attachment 8726066 [details] [diff] [review]
bug1248846.patch

Review of attachment 8726066 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I thought I'd submitted this last week.

Have you managed to get a test build of Firefox working? This code has a lot of syntax errors. It shouldn't run as-is.

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +237,5 @@
>        promise.then(
>          args => {
> +          if (this.unloaded) {
> +            dump("Callback error received after context was destroyed: ${error.message}\n");
> +          } else if (args instanceof SpreadArgs){

The opening curly brace needs a space before it, and the next 4 lines have two extra spaces of indentation. The ESLint tests on our CI server won't accept this formatting.

@@ +239,5 @@
> +          if (this.unloaded) {
> +            dump("Callback error received after context was destroyed: ${error.message}\n");
> +          } else if (args instanceof SpreadArgs){
> +              runSafe(callback, ...args);
> +            else {

This is a syntax error. You're missing a '}'

@@ +246,1 @@
>            }

Another syntax error. Extra closing curly brace.

@@ +246,4 @@
>            }
>          },
>          error => {
> +          if(this.unloaded){

The opening `(` and `{` need spaces before them. ESLint won't accept them as-is.

@@ +252,5 @@
> +              this.withLastError(error, () => {
> +              runSafeSyncWithoutClone(callback);
> +            });
> +          }
> +        }

Another extra closing curly brace.

@@ +253,5 @@
> +              runSafeSyncWithoutClone(callback);
> +            });
> +          }
> +        }
> +    });

This needs another four spaces of indentation.
Attachment #8726066 - Flags: review?(kmaglione+bmo)
Assignee: tparker12 → kmaglione+bmo
Attachment #8737943 - Flags: review?(aswan)
Comment on attachment 8737943 [details]
MozReview Request: Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan

https://reviewboard.mozilla.org/r/44193/#review40859

There are a bunch of tests in this patch, half of which use Cu.reportError() and half of which use dump().  If this is deliberate, a comment somewhere explaning the circumstances in which each is used would be helpful.

::: browser/components/extensions/ext-utils.js:215
(Diff revision 1)
>        case this.DESTROY_EVENT:
>          this.destroy();
>          break;
>  
>        case "DOMWindowCreated":
> +        if (event.target === this.browser.contentDocument) {

I don't have enough context (har har) to understand this change, is it actually connected to the other changes in this patch?

::: toolkit/components/extensions/ExtensionUtils.jsm:81
(Diff revision 1)
>    } catch (e) {
>      Cu.reportError(e);
>      dump(`runSafe failure: cloning into ${context.cloneScope}: ${e}\n\n${filterStack(Error())}`);
>    }
> +  if (context.unloaded) {
> +    dump(`runSafe failure: context is already unloaded ${filterStack(new Error())}\n`);

Other instances are using Cu.reportError(), why is this one using dump() instead?

::: toolkit/components/extensions/ExtensionUtils.jsm:158
(Diff revision 1)
>  
>    get principal() {
>      throw new Error("Not implemented");
>    }
>  
> +  runSafe(...args) {

with all the variants of runSafe flying around in this file, I think it would be easier to follow if this was named runSafeSync to be consistent with the underlying thing it is calling.  likewise with runSafe(Sync)WithoutClone below.

::: toolkit/components/extensions/ExtensionUtils.jsm:159
(Diff revision 1)
>    get principal() {
>      throw new Error("Not implemented");
>    }
>  
> +  runSafe(...args) {
> +    if (this.unloaded) {

this check appears redundant with the one done inside runSafeSync()?

::: toolkit/components/extensions/ExtensionUtils.jsm:309
(Diff revision 1)
>  
>      if (callback) {
>        promise.then(
>          args => {
> -          if (args instanceof SpreadArgs) {
> +          if (this.unloaded) {
> +            dump(`Promise resolved after context unloaded\n`);

with the comment above, this appears to be doubly-redundant (ie, we check it here, this.runSafe also checks it, and runSafeSync() also checks it!)
https://reviewboard.mozilla.org/r/44193/#review40859

> I don't have enough context (har har) to understand this change, is it actually connected to the other changes in this patch?

Only tangentially. It prevents the code from running after the popup was destroyed, which causes a lot of errors similar to the ones from callbacks running too late.

> Other instances are using Cu.reportError(), why is this one using dump() instead?

I used `reportErrors` in all of the synchronous cases, where the caller should be able to know for certain that the context is unloaded before calling, and dump() for the other cases, where it can't. `reportError` causes too much noise for those cases, since it happens a lot for unused results in the short-lived contexts generated by our tests.

> with all the variants of runSafe flying around in this file, I think it would be easier to follow if this was named runSafeSync to be consistent with the underlying thing it is calling.  likewise with runSafe(Sync)WithoutClone below.

My plan was to eventually get rid of those variants, and default to the calls being synchronous, since the promise wrappers have removed most of the need for the asynchronous variants.

> this check appears redundant with the one done inside runSafeSync()?

It is, but it has a more relevant error message.

> with the comment above, this appears to be doubly-redundant (ie, we check it here, this.runSafe also checks it, and runSafeSync() also checks it!)

It's redundant, yes, but the error message is more specific, and the reporting is less severe.
https://reviewboard.mozilla.org/r/44193/#review40859

> My plan was to eventually get rid of those variants, and default to the calls being synchronous, since the promise wrappers have removed most of the need for the asynchronous variants.

I see.  With the explanation this makes sense, but how far out is eventually?  For the sake of other folks reading this code in the mean time, can we keep the names the same (ie add Sync here) for now and then remove Sync everywhere at the same time when that becomes the only flavor?
https://reviewboard.mozilla.org/r/44193/#review40869

::: toolkit/components/extensions/ExtensionUtils.jsm:617
(Diff revision 1)
>      return this.callbacks.has(callback);
>    },
>  
>    fire(...args) {
>      for (let callback of this.callbacks) {
> -      runSafe(this.context, callback, ...args);
> +      Promise.resolve(callback).then(callback => {

I don't think it makes a huge difference, but if you have multiple callbacks, is there a good reason to have multiple promises rather than a single promise where the handler loops through all the callbacks?

::: toolkit/components/extensions/ExtensionUtils.jsm:629
(Diff revision 1)
>      }
>    },
>  
>    fireWithoutClone(...args) {
>      for (let callback of this.callbacks) {
> -      runSafeSyncWithoutClone(callback, ...args);
> +      this.context.runSafeWithoutClone(callback, ...args);

You haven't changed anything here, but I'm wondering why fire() is deferred and fireWithoutClone() is immediate?
https://reviewboard.mozilla.org/r/44193/#review40859

> I see.  With the explanation this makes sense, but how far out is eventually?  For the sake of other folks reading this code in the mean time, can we keep the names the same (ie add Sync here) for now and then remove Sync everywhere at the same time when that becomes the only flavor?

Ideally within the next couple of weeks. I'd rather minimize churn as much as possible here. I don't want to have to rename things twice.
https://reviewboard.mozilla.org/r/44193/#review40869

> I don't think it makes a huge difference, but if you have multiple callbacks, is there a good reason to have multiple promises rather than a single promise where the handler loops through all the callbacks?

The list of registered callbacks might change by the time the promise resolves, and I don't know if new callbacks should be expected to fire for old events.

> You haven't changed anything here, but I'm wondering why fire() is deferred and fireWithoutClone() is immediate?

I was wondering it too. I initially changed `fire` to fire synchronously, but it broke some tests. We're trying to phase out `EventListener`, either way, so I think it's probably not worth bothering about for now.
Comment on attachment 8737943 [details]
MozReview Request: Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan

https://reviewboard.mozilla.org/r/44193/#review40879

I guess that since this code is all about races between callbacks and shutdown, it is probably not practical to test?
Attachment #8737943 - Flags: review+
https://reviewboard.mozilla.org/r/44193/#review40879

I don't think there's any reliable way to test it in mochitests. I may be able to write xpcshell tests for some components of it, though. I'll see if I can make it work.
Review commit: https://reviewboard.mozilla.org/r/46891/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46891/
Attachment #8737943 - Attachment description: MozReview Request: Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r?aswan → MozReview Request: Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan
Attachment #8742007 - Flags: review?(aswan)
Comment on attachment 8737943 [details]
MozReview Request: Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44193/diff/1-2/
https://hg.mozilla.org/integration/fx-team/rev/2a95760bd9cca8d1912a14158380ce1f0d84766d
Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan
Keywords: leave-open
Comment on attachment 8742007 [details]
MozReview Request: Bug 1248846: [webext] Test that event callbacks and promises do not fire later than expected. r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46891/diff/1-2/
Attachment #8737943 - Attachment is obsolete: true
Comment on attachment 8742007 [details]
MozReview Request: Bug 1248846: [webext] Test that event callbacks and promises do not fire later than expected. r?aswan

https://reviewboard.mozilla.org/r/46891/#review43743

The outline of the test seems sensible enough but I think it would benefit from a comment or comments explaining everything.
Also, you're testing promises and event handlers simultaneously, I think that if we ever do have a regression and we're trying to interpret test results, separating those into separate test cases (ie tasks) would make our lives easier.

::: toolkit/components/extensions/ExtensionUtils.jsm:637
(Diff revision 2)
>  
>    close() {
>      if (this.callbacks.size) {
>        this.unregister();
>      }
> -    this.callbacks = null;
> +    this.callbacks = Object.freeze([]);

The description for this patch is about testing, but this looks like an actual functional change?

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:29
(Diff revision 2)
> +    }
> +  };
> +
> +  let fireEvent;
> +  let onEvent = new EventManager(context, "onEvent", fire => {
> +    fireEvent = () => {

nit: this could just be `fireEvent = fire;` ?

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:57
(Diff revision 2)
> +    new Promise(resolve => onSingleton.addListener(resolve)),
> +    context.wrapPromise(Promise.resolve()),
> +    context.wrapPromise(Promise.reject({message: ""})).catch(() => {}),
> +  ];
> +
> +  fireEvent("onEvent");

I'm surprised by this, at this moment you have `fail` attached as a listener.  Is the idea that the caller is guaranteed a chance to run again before the event is actually dispatched and you want to test that that really happens?

::: toolkit/components/extensions/test/xpcshell/test_ext_contexts.js:77
(Diff revision 2)
> +  Promise.resolve("onSingleton").then(fireSingleton);
> +
> +  context.wrapPromise(Promise.resolve("resolved"))
> +         .then(fail);
> +
> +  context.wrapPromise(Promise.reject("rejected"))

nit: the argument should be an Error or an object with a message property
Attachment #8742007 - Flags: review?(aswan)
https://reviewboard.mozilla.org/r/46891/#review43743

> The description for this patch is about testing, but this looks like an actual functional change?

It is, but it's a trivial fix for an issue I only noticed after I updated the test. It just prevents `fire` from throwing if we call it from an asynchronous callback after the context was unloaded.

> nit: this could just be `fireEvent = fire;` ?

Yeah. I originally had it always call `fire` asynchronously, but I realized that that caused problems.

> I'm surprised by this, at this moment you have `fail` attached as a listener.  Is the idea that the caller is guaranteed a chance to run again before the event is actually dispatched and you want to test that that really happens?

The event is dispatched asynchronously after `fire` is called, so if the event listener is removed after `fire` is called but before it's dispatched, we need to not trigger the listener. That's what this tests.
Comment on attachment 8742007 [details]
MozReview Request: Bug 1248846: [webext] Test that event callbacks and promises do not fire later than expected. r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46891/diff/2-3/
Attachment #8742007 - Flags: review?(aswan)
Comment on attachment 8742007 [details]
MozReview Request: Bug 1248846: [webext] Test that event callbacks and promises do not fire later than expected. r?aswan

https://reviewboard.mozilla.org/r/46891/#review43953

Thanks, looks good!
Attachment #8742007 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/fx-team/rev/fc2da6172138c3f1eeaea6a29a3f23be6c2cca7c
Bug 1248846: [webext] Test that event callbacks and promises do not fire later than expected. r=aswan
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.