Closed Bug 1203330 Opened 5 years ago Closed 4 years ago

Convert everything from EventManager to SingletonEventManager

Categories

(WebExtensions :: General, defect, P3)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: billm, Assigned: aswan, Mentored)

References

Details

(Whiteboard: triaged)

Attachments

(4 files, 10 obsolete files)

idle_test.log
438.53 KB, text/x-log
Details
Bug 1203330 Part 1 Fix SingletonEventManager
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1203330 Part 2 Convert EventManager uses to SingletonEventManager
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1203330 Part 3 Remove EventManager
59 bytes, text/x-review-board-request
kmag
: review+
Details 
EventManager is more trouble than it's worth. We should get rid of it.
Mentor: wmccloskey
Blocks: webext
Priority: -- → P3
Flags: blocking-webextensions+
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)
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)
Whiteboard: triaged
Flags: blocking-webextensions+ → blocking-webextensions-
Attached file error.log (obsolete) — 
Mochitest log
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.
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?
Yes I can run the browser and the tests without any problem if I do not apply my changes
Can you attach a patch file with the changes you've made?
Attached patch changes.diff (obsolete) — — Splinter Review 
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.
Attached patch changes.diff (obsolete) — — Splinter Review 
Test ok, will go on with the other files
Attachment #8765975 - Attachment is obsolete: true
Attached patch changes.diff (obsolete) — — Splinter Review 
All is ported except ext-test.js who still have broken tests
Attachment #8766963 - Attachment is obsolete: true
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.
Component: WebExtensions: Untriaged → WebExtensions: General
Flags: blocking-webextensions- → needinfo?(edoardo.putti)
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)
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.
Attached file idle_test.log — 
Error log for idle test
Attachment #8765633 - Attachment is obsolete: true
I'm really suspicious of the long idle timing, will update the patch briefly
Attached patch changes.diff (obsolete) — — Splinter Review
Attachment #8767193 - Attachment is obsolete: true
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)
Attached patch improve SingletonEventManager (obsolete) — — Splinter Review 
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)
Attached patch Edoardo's patch (obsolete) — — Splinter Review 
Here is Edoardo's patch, fixed up to use the new SingletonEventManager stuff.
Attachment #8795435 - Attachment is obsolete: true
Attachment #8806538 - Flags: review+
Attached patch browser changes (obsolete) — — Splinter Review 
This patch fixes up the uses of EventManager in browser/.
Attachment #8806540 - Flags: review?(kmaglione+bmo)
Attached patch convert WindowEventManager (obsolete) — — Splinter Review 
This makes WindowEventManager inherit from SingletonEventManager.
Attachment #8806541 - Flags: review?(kmaglione+bmo)
Attached patch remove EventManager (obsolete) — — Splinter Review 
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)
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 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+
Attachment #8806540 - Flags: review?(kmaglione+bmo) → review+
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+
Attachment #8806542 - Flags: review?(kmaglione+bmo) → review+
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)
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)
Attachment #8806537 - Attachment is obsolete: true
Attachment #8806538 - Attachment is obsolete: true
Attachment #8806540 - Attachment is obsolete: true
Attachment #8806541 - Attachment is obsolete: true
Attachment #8806542 - Attachment is obsolete: true
Assignee: nobody → aswan
Attachment #8830522 - Flags: review?(kmaglione+bmo)
Attachment #8830523 - Flags: review?(kmaglione+bmo)
Attachment #8830524 - Flags: review?(kmaglione+bmo)
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 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 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 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 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.)
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 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).
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
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.