Closed
Bug 1203330
Opened 6 years ago
Closed 4 years ago
Convert everything from EventManager to SingletonEventManager
Categories
(WebExtensions :: General, defect, P3)
WebExtensions
General
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: billm, Assigned: aswan, Mentored)
References
Details
(Whiteboard: triaged)
Attachments
(4 files, 10 obsolete files)
EventManager is more trouble than it's worth. We should get rid of it.
Reporter | ||
Updated•6 years ago
|
Mentor: wmccloskey
Updated•5 years ago
|
Flags: blocking-webextensions+
Comment 1•5 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?
Flags: needinfo?(wmccloskey)
Reporter | ||
Comment 2•5 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.
Flags: needinfo?(wmccloskey)
Updated•5 years ago
|
Whiteboard: triaged
Updated•5 years ago
|
Flags: blocking-webextensions+ → blocking-webextensions-
Comment 3•5 years ago
|
||
Mochitest log
Comment 4•5 years 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•5 years 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•5 years ago
|
||
Yes I can run the browser and the tests without any problem if I do not apply my changes
Assignee | ||
Comment 7•5 years ago
|
||
Can you attach a patch file with the changes you've made?
Comment 8•5 years ago
|
||
Assignee | ||
Comment 9•5 years 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•5 years ago
|
||
Test ok, will go on with the other files
Attachment #8765975 -
Attachment is obsolete: true
Comment 11•5 years ago
|
||
All is ported except ext-test.js who still have broken tests
Attachment #8766963 -
Attachment is obsolete: true
Assignee | ||
Comment 12•5 years 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•5 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: General
Flags: blocking-webextensions- → needinfo?(edoardo.putti)
Comment 13•5 years 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()
Flags: needinfo?(edoardo.putti)
Comment 14•5 years 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 16•5 years ago
|
||
I'm really suspicious of the long idle timing, will update the patch briefly
Comment 17•5 years ago
|
||
Attachment #8767193 -
Attachment is obsolete: true
Comment 18•4 years 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.
Flags: needinfo?(wmccloskey)
Reporter | ||
Comment 19•4 years ago
|
||
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.
Flags: needinfo?(wmccloskey)
Attachment #8806537 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Comment 20•4 years ago
|
||
Here is Edoardo's patch, fixed up to use the new SingletonEventManager stuff.
Attachment #8795435 -
Attachment is obsolete: true
Attachment #8806538 -
Flags: review+
Reporter | ||
Comment 21•4 years ago
|
||
This patch fixes up the uses of EventManager in browser/.
Attachment #8806540 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Comment 22•4 years ago
|
||
This makes WindowEventManager inherit from SingletonEventManager.
Attachment #8806541 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Comment 23•4 years ago
|
||
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).
Attachment #8806542 -
Flags: review?(kmaglione+bmo)
Reporter | ||
Comment 24•4 years 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•4 years 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.
Attachment #8806537 -
Flags: review?(kmaglione+bmo) → review+
Updated•4 years ago
|
Attachment #8806540 -
Flags: review?(kmaglione+bmo) → review+
Comment 26•4 years 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?
Attachment #8806541 -
Flags: review?(kmaglione+bmo) → review+
Updated•4 years ago
|
Attachment #8806542 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 27•4 years 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...)
Flags: needinfo?(wmccloskey)
Reporter | ||
Comment 28•4 years 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.
Flags: needinfo?(wmccloskey)
Assignee | ||
Updated•4 years ago
|
Attachment #8806537 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8806538 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8806540 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8806541 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8806542 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → aswan
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Attachment #8830522 -
Flags: review?(kmaglione+bmo)
Attachment #8830523 -
Flags: review?(kmaglione+bmo)
Attachment #8830524 -
Flags: review?(kmaglione+bmo)
Comment 32•4 years 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(...)`
Attachment #8830522 -
Flags: review?(kmaglione+bmo) → review+
Comment 33•4 years ago
|
||
mozreview-review |
Comment on attachment 8830523 [details] Bug 1203330 Part 2 Convert EventManager uses to SingletonEventManager https://reviewboard.mozilla.org/r/107280/#review108428
Attachment #8830523 -
Flags: review?(kmaglione+bmo) → review+
Comment 34•4 years ago
|
||
mozreview-review |
Comment on attachment 8830524 [details] Bug 1203330 Part 3 Remove EventManager https://reviewboard.mozilla.org/r/107282/#review108430
Attachment #8830524 -
Flags: review?(kmaglione+bmo) → review+
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•4 years 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•4 years 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•4 years 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•4 years 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•4 years 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•4 years 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•4 years 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•4 years 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
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•3 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•