Bugzilla

Updated

•

4 years ago

Blocks: 1214433

Priority: -- → P3

Bill McCloskey [inactive unless it's an emergency] (:billm)

Updated

•

4 years ago

Flags: blocking-webextensions+

Shubheksha Jalan

Comment 1

•

3 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

•

3 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)

:shell escalante

Updated

•

3 years ago

Whiteboard: triaged

Updated

•

3 years ago

Flags: blocking-webextensions+ → blocking-webextensions-

Comment 3

•

3 years ago

Posted file error.log (obsolete) — Details

Mochitest log

Comment 4

•

3 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

•

3 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

•

3 years ago

Yes I can run the browser and the tests without any problem if I do not apply my changes

(Assignee)

Comment 7

•

3 years ago

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

Comment 8

•

3 years ago

Posted patch changes.diff (obsolete) — Details — Splinter Review

(Assignee)

Comment 9

•

3 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

•

3 years ago

Posted patch changes.diff (obsolete) — Details — Splinter Review

Test ok, will go on with the other files

Attachment #8765975 - Attachment is obsolete: true

Comment 11

•

3 years ago

Posted patch changes.diff (obsolete) — Details — Splinter Review

All is ported except ext-test.js who still have broken tests

Attachment #8766963 - Attachment is obsolete: true

(Assignee)

Comment 12

•

3 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

•

3 years ago

Component: WebExtensions: Untriaged → WebExtensions: General

Flags: blocking-webextensions- → needinfo?(edoardo.putti)

Bob Silverberg [:bsilverberg]

Comment 13

•

3 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

•

3 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 15

•

3 years ago

Posted file idle_test.log — Details

Error log for idle test

Attachment #8765633 - Attachment is obsolete: true

Comment 16

•

3 years ago

I'm really suspicious of the long idle timing, will update the patch briefly

Comment 17

•

3 years ago

Posted patch changes.diff (obsolete) — Details — Splinter Review

Attachment #8767193 - Attachment is obsolete: true

Bill McCloskey [inactive unless it's an emergency] (:billm)

Comment 18

•

2 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

•

2 years ago

Posted patch improve SingletonEventManager (obsolete) — Details — 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)

Bill McCloskey [inactive unless it's an emergency] (:billm)

(Reporter)

Comment 20

•

2 years ago

Posted patch Edoardo's patch (obsolete) — Details — 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+

Bill McCloskey [inactive unless it's an emergency] (:billm)

(Reporter)

Comment 21

•

2 years ago

Posted patch browser changes (obsolete) — Details — Splinter Review

This patch fixes up the uses of EventManager in browser/.

Attachment #8806540 - Flags: review?(kmaglione+bmo)

Bill McCloskey [inactive unless it's an emergency] (:billm)

(Reporter)

Comment 22

•

2 years ago

Posted patch convert WindowEventManager (obsolete) — Details — Splinter Review

This makes WindowEventManager inherit from SingletonEventManager.

Attachment #8806541 - Flags: review?(kmaglione+bmo)

Bill McCloskey [inactive unless it's an emergency] (:billm)

(Reporter)

Comment 23

•

2 years ago

Posted patch remove EventManager (obsolete) — Details — 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)

Bill McCloskey [inactive unless it's an emergency] (:billm)

(Reporter)

Comment 24

•

2 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

•

2 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

•

2 years ago

Attachment #8806540 - Flags: review?(kmaglione+bmo) → review+

Comment 26

•

2 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

•

2 years ago

Attachment #8806542 - Flags: review?(kmaglione+bmo) → review+

Bill McCloskey [inactive unless it's an emergency] (:billm)

(Assignee)

Comment 27

•

2 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

•

2 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

•

2 years ago

Attachment #8806537 - Attachment is obsolete: true

(Assignee)

Updated

•

2 years ago

Attachment #8806538 - Attachment is obsolete: true

(Assignee)

Updated

•

2 years ago

Attachment #8806540 - Attachment is obsolete: true

(Assignee)

Updated

•

2 years ago

Attachment #8806541 - Attachment is obsolete: true

(Assignee)

Updated

•

2 years ago

Attachment #8806542 - Attachment is obsolete: true

(Assignee)

Updated

•

2 years ago

Assignee: nobody → aswan

Comment hidden (mozreview-request)

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.

Existing uses of SingletonEventManager are all converted to async calls.
Note that some of them were previously synchronous, but it didn't appear
to be necessary.

Also added a callOnClose for SingletonEventManager when the last listener
is removed.

Review commit: https://reviewboard.mozilla.org/r/107278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107278/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/107280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107280/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/107282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107282/

(Assignee)

Updated

•

2 years ago

Attachment #8830522 - Flags: review?(kmaglione+bmo)

Attachment #8830523 - Flags: review?(kmaglione+bmo)

Attachment #8830524 - Flags: review?(kmaglione+bmo)

Comment 32

•

2 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

•

2 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

•

2 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 on attachment 8830522 [details]
Bug 1203330 Part 1 Fix SingletonEventManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107278/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8830523 [details]
Bug 1203330 Part 2 Convert EventManager uses to SingletonEventManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107280/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8830524 [details]
Bug 1203330 Part 3 Remove EventManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107282/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8830522 [details]
Bug 1203330 Part 1 Fix SingletonEventManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107278/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8830523 [details]
Bug 1203330 Part 2 Convert EventManager uses to SingletonEventManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107280/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8830524 [details]
Bug 1203330 Part 3 Remove EventManager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107282/diff/2-3/

Matthew Wein [:mattw]

Comment 41

•

2 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

•

2 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

•

2 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...)