Convert everything from EventManager to SingletonEventManager

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
WebExtensions: General
P3
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: billm, Assigned: aswan, Mentored)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 10 obsolete attachments)

438.53 KB, text/x-log
Details
59 bytes, text/x-review-board-request
kmag
: review+
Details | Review
59 bytes, text/x-review-board-request
kmag
: review+
Details | Review
59 bytes, text/x-review-board-request
kmag
: review+
Details | Review
(Reporter)

Description

2 years ago
EventManager is more trouble than it's worth. We should get rid of it.
(Reporter)

Updated

2 years ago
Depends on: 1203331
(Reporter)

Updated

2 years ago
Mentor: wmccloskey@mozilla.com

Updated

2 years ago
Blocks: 1214433
Priority: -- → P3

Updated

2 years ago
Flags: blocking-webextensions+

Comment 1

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?
Flags: needinfo?(wmccloskey)
(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.
Flags: needinfo?(wmccloskey)

Updated

a year ago
Whiteboard: triaged

Updated

a year ago
Flags: blocking-webextensions+ → blocking-webextensions-

Comment 3

11 months ago
Created attachment 8765633 [details]
error.log

Mochitest log

Comment 4

11 months 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

11 months 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

11 months ago
Yes I can run the browser and the tests without any problem if I do not apply my changes
(Assignee)

Comment 7

11 months ago
Can you attach a patch file with the changes you've made?

Comment 8

11 months ago
Created attachment 8765975 [details] [diff] [review]
changes.diff
(Assignee)

Comment 9

11 months 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.
Created attachment 8766963 [details] [diff] [review]
changes.diff

Test ok, will go on with the other files
Attachment #8765975 - Attachment is obsolete: true
Created attachment 8767193 [details] [diff] [review]
changes.diff

All is ported except ext-test.js who still have broken tests
Attachment #8766963 - Attachment is obsolete: true
(Assignee)

Comment 12

11 months 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

8 months ago
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.
Created attachment 8795429 [details]
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
Created attachment 8795435 [details] [diff] [review]
changes.diff
Attachment #8767193 - Attachment is obsolete: true

Comment 18

7 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.
Flags: needinfo?(wmccloskey)
(Reporter)

Comment 19

7 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.
Flags: needinfo?(wmccloskey)
Attachment #8806537 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 20

7 months ago
Created attachment 8806538 [details] [diff] [review]
Edoardo's patch

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

7 months ago
Created attachment 8806540 [details] [diff] [review]
browser changes

This patch fixes up the uses of EventManager in browser/.
Attachment #8806540 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 22

7 months ago
Created attachment 8806541 [details] [diff] [review]
convert WindowEventManager

This makes WindowEventManager inherit from SingletonEventManager.
Attachment #8806541 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 23

7 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).
Attachment #8806542 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 24

7 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 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+
(Assignee)

Comment 27

4 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...)
Flags: needinfo?(wmccloskey)
(Reporter)

Comment 28

4 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.
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

4 months ago
Attachment #8806537 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8806538 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8806540 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8806541 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Attachment #8806542 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Assignee: nobody → aswan
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
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 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 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 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 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

4 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

4 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
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 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

4 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
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.