Closed
Bug 1248846
Opened 8 years ago
Closed 8 years ago
Make sure event handlers and callbacks are not called after a context is destroyed
Categories
(WebExtensions :: Untriaged, defect, P3)
WebExtensions
Untriaged
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla48
People
(Reporter: kmag, Assigned: kmag, Mentored)
References
Details
(Whiteboard: [good first bug] triaged)
Attachments
(3 files, 2 obsolete files)
4.30 KB,
patch
|
Details | Diff | Splinter Review | |
3.86 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
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.
Assignee | ||
Updated•8 years ago
|
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.
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
Attachment #8722251 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Comment 6•8 years ago
|
||
Attachment #8723370 -
Flags: review?(kmaglione+bmo)
Updated•8 years ago
|
Assignee: nobody → tparker12
Priority: -- → P3
Whiteboard: [good first bug] → [good first bug] triaged
Assignee | ||
Updated•8 years ago
|
Attachment #8722251 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
Another attempt at fixing the syntax errors and folded the changes with the last patch as well.
Attachment #8726066 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 9•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: tparker12 → kmaglione+bmo
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/44193/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/44193/
Attachment #8737943 -
Flags: review?(aswan)
Updated•8 years ago
|
Attachment #8737943 -
Flags: review?(aswan)
Comment 11•8 years ago
|
||
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!)
Assignee | ||
Comment 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
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?
Comment 14•8 years ago
|
||
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?
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Assignee | ||
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
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/
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2a95760bd9cca8d1912a14158380ce1f0d84766d Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 22•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8737943 -
Attachment is obsolete: true
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2a95760bd9cc
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
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.
Assignee | ||
Comment 26•8 years ago
|
||
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 27•8 years ago
|
||
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+
Assignee | ||
Comment 28•8 years ago
|
||
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
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc2da6172138
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•