| Submitter | Diff | Changes | Open Issues | Last Updated |
|---|---|---|---|---|
| Loading... | ||||
| Error loading review requests: | ||||
EventManager is more trouble than it's worth. We should get rid of it.
|
(Reporter) | |
Updated•2 years ago
|
||
|
||
Updated•2 years ago
|
||
I'm a beginner and would like to take this up. Can I get some more information regarding what needs to be done here?
|
|
(Reporter) | |
Comment 2•2 years ago
|
||
Hi sj, Have you built Firefox before? If not, you can read instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions. The basic idea of this bug is to convert uses of EventManager to SingletonEventManager. You can see all the uses of both of these here: https://dxr.mozilla.org/mozilla-central/search?q=EventManager+path%3Atoolkit%2Fcomponents%2Fextensions&redirect=true&case=false There is documentation for EventManager here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#282 And for SingletonEventManager here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#375 It might be good to break this up and convert only a few uses at a time. For example, the one here is pretty simple: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-storage.js#44 You should just be able to change "EventManager" to "SingletonEventManager". Then you can run tests like this: mach mochitest toolkit/components/extensions Let me know how that goes and then we can try to do some more.
|
||
Updated•2 years ago
|
||
|
|
||
Updated•2 years ago
|
||
|
||
Comment 3•a year ago
|
||
Created attachment 8765633 [details]
error.log
Mochitest log|
|
||
Comment 4•a year ago
|
||
I'm interested in the bug but I cannot get a useful log from the test runner. Running mach mochitest toolkit/components/extensions/test/mochitest/test_ext_storage.html --log-mach=error.log --log-mach-level DEBUG doesn't provide any useful information. The process just hangs and I have to stop it with Ctrl + C My last attempt changed occurrence of EventManager to SingleEventManager but I did not change anything else.
|
(Assignee) | |
Comment 5•a year ago
|
||
Edoardo, it looks like you've got a bad build. Can you do "mach run" from your build directory and use the browser? Can you run something like "mach mochitest toolkit/components/extensions/test/mochitest" without any of your changes applied?
|
|
||
Comment 6•a year ago
|
||
Yes I can run the browser and the tests without any problem if I do not apply my changes
|
|
(Assignee) | |
Comment 7•a year ago
|
||
Can you attach a patch file with the changes you've made?
|
|
||
Comment 8•a year ago
|
||
Created attachment 8765975 [details] [diff] [review] changes.diff
|
|
(Assignee) | |
Comment 9•a year ago
|
||
Okay, first: your changes are now causing the test to fail in a way that they time out rather than failing immediately. The default timeout is something like 5 minutes but you interrupted it in less than a minute.
Still, when I ran with your changes I did get some immediate output (before getting into the state where it was waiting for a timeout):
0 INFO SimpleTest START
1 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_ext_storage.html
2 INFO Error: Unable to restore focus, expect failures and timeouts.
3 INFO SpawnTask.js | Entering test test_backgroundScript
4 INFO Extension loaded
5 INFO TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_storage.html | storage is local - Expected: local, Actual: local
6 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_storage.html | Error: TypeError: obj2[prop] is undefined :: checkSub@moz-extension://1569e74c-87b0-614f-859f-a618a27b920a/%7Bc9a4b7ab-cae6-6c47-818a-ebda9e583be9%7D.js:28:9
checkChanges@moz-extension://1569e74c-87b0-614f-859f-a618a27b920a/%7Bc9a4b7ab-cae6-6c47-818a-ebda9e583be9%7D.js:33:5
@moz-extension://1569e74c-87b0-614f-859f-a618a27b920a/%7Bc9a4b7ab-cae6-6c47-818a-ebda9e583be9%7D.js:42:5
promise callback*backgroundScript@moz-extension://1569e74c-87b0-614f-859f-a618a27b920a/%7Bc9a4b7ab-cae6-6c47-818a-ebda9e583be9%7D.js:41:3
@moz-extension://1569e74c-87b0-614f-859f-a618a27b920a/%7Bc9a4b7ab-cae6-6c47-818a-ebda9e583be9%7D.js:1:11
testHandler@SimpleTest/ExtensionTestUtils.js:61:7
testResult@SimpleTest/ExtensionTestUtils.js:71:36
7 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_storage.html | storage
testResult@SimpleTest/ExtensionTestUtils.js:68:9
I think the specific reason your tests are failing is that the `fire` callback that EventManager provides uses Context.runSafe (https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#655). SingletonEventManager doesn't do that, you'll need to call it yourself from the event handler.|
|
||
Comment 10•a year ago
|
||
Created attachment 8766963 [details] [diff] [review] changes.diff Test ok, will go on with the other files
|
|
||
Comment 11•a year ago
|
||
Created attachment 8767193 [details] [diff] [review] changes.diff All is ported except ext-test.js who still have broken tests
|
|
(Assignee) | |
Comment 12•a year ago
|
||
Hi Edoardo, Are you waiting for feedback on this? If you are, its a good practice to set a flag on the bug or one of its attachments to make sure that somebody notices. There are a few possibilities: - If you think the patch is ready to be reviewed, set the r? flag on the attachment - If the patch isn't quite ready for final review but you would like somebody to look at it and give you feedback, set the r? flag on the attachment - If you have other more general questions, set the needinfo? flag on the bug All of those flags also have to be directed to a specific person, for this bug feel free to set them to either me or Kris Maglione.
|
|
||
Updated•10 months ago
|
||
|
|
||
Comment 13•10 months ago
|
||
Hello all,
I've pulled from central and I see there have been changes regarding the webextension, should I be worried?
My problem is porting ext-test.js,
onMessage: new SingletonEventManager(context, "test.onMessage", fire => {
let handlers = messageHandlers.get(extension);
handlers.add(fire);
return () => {
handlers.delete(fire);
};
}).api().
I did the obvious work and now it is this
onMessage: new SingletonEventManager(context, "test.onMessage", fire => {
let callback = a => {
context.runSafe(fire)
}
let handlers = messageHandlers.get(extension);
handlers.add(callback);
return () => {
handlers.delete(callback);
};
}).api().
I even tried wrapping the context.runSafe call into a Promise
Promise(fire).then(fire => {
context.runSafe(fire);
});
but it hangs on the test_chrome_ext_idle.html
The log shows that the test set a listener
INFO idleTime: 4688, detectionInterval: 15
INFO Listener added
but it is a time so big it must be a wrong call. Reloading the test keep calling the listener with shorter time
Extension loaded
idleTime: 122, detectionInterval: 15
Listener added
SpawnTask.js | Entering test testWithRealIdleService
Extension loaded
idleTime: 169, detectionInterval: 15
Listener added
I let it run as long as it needed and it fails with the message
Failed to retrieve MOZ_UPLOAD_DIR env var
100 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html | Test timed out.
reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:114:7
TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:135:7
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
TestRunner.runTests@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:367:5
RunSet.runtests@chrome://mochikit/content/tests/SimpleTest/setup.js:186:3
RunSet.runall@chrome://mochikit/content/tests/SimpleTest/setup.js:165:5
hookupTests@chrome://mochikit/content/tests/SimpleTest/setup.js:258:5
parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
hookup@chrome://mochikit/content/tests/SimpleTest/setup.js:238:5
linkAndHookup@chrome://mochikit/content/harness.xul:59:3
parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
getTestList@chrome://mochikit/content/chrome-harness.js:260:3
loadTests@chrome://mochikit/content/harness.xul:38:3
EventListener.handleEvent*@chrome://mochikit/content/harness.xul:62:5
101 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html | Extension left running at test shutdown
ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:107:7
SimpleTest.finish/executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1154:19
SimpleTest.finish@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1167:5
killTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:123:7
delayedKillTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:150:47
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:150:7
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
TestRunner.runTests@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:367:5
RunSet.runtests@chrome://mochikit/content/tests/SimpleTest/setup.js:186:3
RunSet.runall@chrome://mochikit/content/tests/SimpleTest/setup.js:165:5
hookupTests@chrome://mochikit/content/tests/SimpleTest/setup.js:258:5
parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
hookup@chrome://mochikit/content/tests/SimpleTest/setup.js:238:5
linkAndHookup@chrome://mochikit/content/harness.xul:59:3
parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
getTestList@chrome://mochikit/content/chrome-harness.js:260:3
loadTests@chrome://mochikit/content/harness.xul:38:3
EventListener.handleEvent*@chrome://mochikit/content/harness.xul:62:5
102 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html | no tasks awaiting on messages - got "[\"listenerFired\"]", expected "[]"
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:270:5
ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:29:7
SimpleTest.finish/executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1154:19
Async*SimpleTest.finish/executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1160:13
SimpleTest.finish@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1167:5
killTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:123:7
delayedKillTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:150:47
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:150:7
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
setTimeout handler*TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:156:5
TestRunner.runTests@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:367:5
RunSet.runtests@chrome://mochikit/content/tests/SimpleTest/setup.js:186:3
RunSet.runall@chrome://mochikit/content/tests/SimpleTest/setup.js:165:5
hookupTests@chrome://mochikit/content/tests/SimpleTest/setup.js:258:5
parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
hookup@chrome://mochikit/content/tests/SimpleTest/setup.js:238:5
linkAndHookup@chrome://mochikit/content/harness.xul:59:3
parseTestManifest@chrome://mochikit/content/manifestLibrary.js:38:5
getTestManifest/req.onload@chrome://mochikit/content/manifestLibrary.js:49:11
EventHandlerNonNull*getTestManifest@chrome://mochikit/content/manifestLibrary.js:45:3
getTestList@chrome://mochikit/content/chrome-harness.js:260:3
loadTests@chrome://mochikit/content/harness.xul:38:3
EventListener.handleEvent*@chrome://mochikit/content/harness.xul:62:5
MEMORY STAT | vsize 1068MB | residentFast 279MB | heapAllocated 89MB
103 INFO TEST-OK | toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html | took 308130ms
104 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html | Extension left running at test shutdown - Result logged after SimpleTest.finish()
105 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_idle.html | no tasks awaiting on messages - Result logged after SimpleTest.finish()
|
||
Comment 14•10 months ago
|
||
I think the problem you are having is with that specific test, as it expects the system to remain idle for 15 seconds. If you interact with your computer at all while it is waiting for that 15 seconds to elapse, that will reset the idle time and therefore it will continue to wait, which is likely why it is timing out. Please try running the test and do not interact with your computer while it is running. Hopefully this will fix your issue.
|
|
||
Comment 15•10 months ago
|
||
Created attachment 8795429 [details]
idle_test.log
Error log for idle test|
|
||
Comment 16•10 months ago
|
||
I'm really suspicious of the long idle timing, will update the patch briefly
|
|
||
Comment 17•10 months ago
|
||
Created attachment 8795435 [details] [diff] [review] changes.diff
|
|
||
Comment 18•9 months ago
|
||
I think Edoardo might want some feedback on this patch Bill, but not that the review flag didn't get set. Edoardo, please set billm to review the patch so that he'll get notification.
|
|
(Reporter) | |
Comment 19•9 months ago
|
||
Created attachment 8806537 [details] [diff] [review] improve SingletonEventManager This patch adds the ability to run SingletonEventManager handlers in different modes: sync, async, raw (no exception handling, arg cloning, or asynchrony), or asyncWithoutClone. When you call the handler, you're required to specify which variant you want. For the existing uses of SingletonEventManager, I converted everything to async calls. Many of them are currently sync, but I don't think there's any reason for this. I also added a callOnClose for SingletonEventManager when you remove the last listener. Otherwise these things could be a source of memory leaks. I think we can get an unbounded number of them with Ports.
|
|
(Reporter) | |
Comment 20•9 months ago
|
||
Created attachment 8806538 [details] [diff] [review] Edoardo's patch Here is Edoardo's patch, fixed up to use the new SingletonEventManager stuff.
|
|
(Reporter) | |
Comment 21•9 months ago
|
||
Created attachment 8806540 [details] [diff] [review] browser changes This patch fixes up the uses of EventManager in browser/.
|
|
(Reporter) | |
Comment 22•9 months ago
|
||
Created attachment 8806541 [details] [diff] [review] convert WindowEventManager This makes WindowEventManager inherit from SingletonEventManager.
|
|
(Reporter) | |
Comment 23•9 months ago
|
||
Created attachment 8806542 [details] [diff] [review] remove EventManager This patch removes EventManager. At some point we should s/SingletonEventManager/EventManager/g. I think that could be a follow-up bug. We should also try to remove some of the runSafe variants that we no longer need (or at least stop exposing them from ExtensionUtils).
|
|
(Reporter) | |
Comment 24•9 months ago
|
||
Also, thanks Edoardo! This bug turned out to be harder than I thought. I assumed SingletonEventManager had all the functionality of EventManager, but it doesn't.
|
||
Comment 25•9 months ago
|
||
Comment on attachment 8806537 [details] [diff] [review] improve SingletonEventManager Review of attachment 8806537 [details] [diff] [review]: ----------------------------------------------------------------- I like this. Thanks ::: toolkit/components/extensions/ExtensionUtils.jsm @@ +1034,5 @@ > > + let fire = { > + sync: (...args) => { > + if (shouldFire()) { > + return runSafeSync(this.context, callback, ...args); Please use `context.runSafe` instead. I know it does some redundant sanity checks, but I'd still rather we use it as consistently as possible. @@ +1040,5 @@ > + }, > + async: (...args) => { > + Promise.resolve().then(() => { > + if (shouldFire()) { > + runSafeSync(this.context, callback, ...args); Same, please use `context.runSafe` @@ +1045,5 @@ > + } > + }); > + }, > + raw: (...args) => { > + if (shouldFire()) { Hm. I guess this is the same behavior as before, but it seems kind of weird that we silently skip the call here... Can we maybe add the `shouldFire` function as a method of `fire`, and require that callers of `.raw()` check it first? That should already be handled for the Messenger.onMessage case by the context.active check. Then we can make this an assertion instead of a conditional. @@ +1063,1 @@ > this.unregister.set(callback, unregister); Hm. It occurs to me that we should check whether `callback` is already in `unregister`, and if it is, either bail before calling `register`, or call the existing `unregister` function first. Otherwise, if someone calls `addListener` twice for the same callback, there won't be any way to actually remove it. @@ +1063,4 @@ > this.unregister.set(callback, unregister); > + > + if (this.unregister.size == 1) { > + this.context.callOnClose(this); `callOnClose` can safely be called multiple times for the same object, so the size check shouldn't be necessary. If we do keep it, though, I'd rather we check `size == 0` before adding the callback, so it works correctly for the case where the same listener is added twice.
|
|
||
Updated•9 months ago
|
||
|
|
||
Comment 26•9 months ago
|
||
Comment on attachment 8806541 [details] [diff] [review] convert WindowEventManager Review of attachment 8806541 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/extensions/ext-utils.js @@ +1166,3 @@ > // add/removeEventListener on each XUL window. > global.WindowEventManager = function(context, name, event, listener) { > + SingletonEventManager.call(this, context, name, fire => { Can we maybe convert this to an ES6 class that subclasses SingletonEventManager, while we're here?
|
|
||
Updated•9 months ago
|
||
|
|
(Assignee) | |
Comment 27•6 months ago
|
||
Hey Bill, did this fall of your radar? Can we land it? (If you've got your hands full with other things these days, I can pick it up and bring it up to date as necessary...)
|
|
(Reporter) | |
Comment 28•6 months ago
|
||
Yeah, I had a burst of energy to do this and now it's gone. I'd appreciate it if you could pick this up. The patches here are the latest I have. I never got a chance to address Kris's review feedback.
|
|
(Assignee) | |
Updated•6 months ago
|
||
|
|
(Assignee) | |
Updated•6 months ago
|
||
|
|
(Assignee) | |
Updated•6 months ago
|
||
|
|
(Assignee) | |
Updated•6 months ago
|
||
|
|
(Assignee) | |
Updated•6 months ago
|
||
|
|
(Assignee) | |
Updated•6 months ago
|
||
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
(Assignee) | |
Updated•6 months ago
|
||
|
|
||
Comment 32•6 months ago
|
||
| mozreview-review | ||
Comment on attachment 8830522 [details] Bug 1203330 Part 1 Fix SingletonEventManager https://reviewboard.mozilla.org/r/107278/#review108420 ::: toolkit/components/extensions/ExtensionUtils.jsm:725 (Diff revision 1) > + Promise.resolve().then(() => { > + if (shouldFire()) { > + this.context.runSafe(callback, ...args); We should probably return this, and the outer promise, in case we have use for it in the future. Same in the withoutClone version. ::: toolkit/components/extensions/ExtensionUtils.jsm:740 (Diff revision 1) > return callback(...args); > + }, > + asyncWithoutClone: (...args) => { > + Promise.resolve().then(() => { > + if (shouldFire()) { > + runSafeSyncWithoutClone(callback, ...args); `context.runSafeWithoutClone(...)`
|
|
||
Comment 33•6 months ago
|
||
| mozreview-review | ||
Comment on attachment 8830523 [details] Bug 1203330 Part 2 Convert EventManager uses to SingletonEventManager https://reviewboard.mozilla.org/r/107280/#review108428
|
|
||
Comment 34•6 months ago
|
||
| mozreview-review | ||
Comment on attachment 8830524 [details] Bug 1203330 Part 3 Remove EventManager https://reviewboard.mozilla.org/r/107282/#review108430
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 41•6 months ago
|
||
| mozreview-review | ||
Comment on attachment 8830522 [details] Bug 1203330 Part 1 Fix SingletonEventManager https://reviewboard.mozilla.org/r/107278/#review108474 ::: toolkit/components/extensions/ExtensionUtils.jsm:749 (Diff revisions 1 - 3) > if (shouldFire()) { > - runSafeSyncWithoutClone(callback, ...args); > + return this.context.runSafeWithoutClone(callback, ...args); > } > }); > + > + // make sure we catch errors in case the caller does not Just a small drive-by nit: could you make these comments into proper sentences before landing?
|
|
||
Comment 42•6 months ago
|
||
| mozreview-review | ||
Comment on attachment 8830522 [details] Bug 1203330 Part 1 Fix SingletonEventManager https://reviewboard.mozilla.org/r/107278/#review108476 ::: toolkit/components/extensions/ExtensionUtils.jsm:750 (Diff revisions 1 - 3) > - runSafeSyncWithoutClone(callback, ...args); > + return this.context.runSafeWithoutClone(callback, ...args); > } > }); > + > + // make sure we catch errors in case the caller does not > + promise.catch(err => {}); This will just hide the unchecked rejection message, which we don't want. Just return the original promise. (Same for above.)
|
|
(Assignee) | |
Comment 43•6 months ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8830522 [details] Bug 1203330 Part 1 Fix SingletonEventManager https://reviewboard.mozilla.org/r/107278/#review108476 > This will just hide the unchecked rejection message, which we don't want. Just return the original promise. (Same for above.) But we have a bunch of callers that call `fire.async()` and ignore the return value. I can change them all to catch and ignore errors at the call sites or we can just do it here. (callers can still add their own error handlers if they really care...)
|
|
||
Comment 44•6 months ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8830522 [details] Bug 1203330 Part 1 Fix SingletonEventManager https://reviewboard.mozilla.org/r/107278/#review108476 > But we have a bunch of callers that call `fire.async()` and ignore the return value. I can change them all to catch and ignore errors at the call sites or we can just do it here. (callers can still add their own error handlers if they really care...) We don't want to ignore errors. This change just prevents the unchecked rejection warning from being displayed (which we don't want). It doesn't prevent the promise from being rejected (which we also don't want).
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 51•6 months ago
|
||
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/31fac390e15d Part 1 Fix SingletonEventManager r=kmag https://hg.mozilla.org/integration/autoland/rev/3a12c51c3eca Part 2 Convert EventManager uses to SingletonEventManager r=kmag https://hg.mozilla.org/integration/autoland/rev/2d42350d209a Part 3 Remove EventManager r=kmag
|
||
Comment 52•6 months ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/0a15564434ca for https://treeherder.mozilla.org/logviewer.html#?job_id=72411702&repo=autoland
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 56•6 months ago
|
||
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/245f6ee1f9bd Part 1 Fix SingletonEventManager r=kmag https://hg.mozilla.org/integration/autoland/rev/f47fbdf0d724 Part 2 Convert EventManager uses to SingletonEventManager r=kmag https://hg.mozilla.org/integration/autoland/rev/e1a52fab45f3 Part 3 Remove EventManager r=kmag
|
||
Comment 57•6 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/245f6ee1f9bd https://hg.mozilla.org/mozilla-central/rev/f47fbdf0d724 https://hg.mozilla.org/mozilla-central/rev/e1a52fab45f3
Description
•