1095267 - Remove Task.jsm dependency on Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Alex Vincent [:WeirdAl]]

Can we please remove Promise.jsm and Promise.defer() from Task.jsm?

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: bug1095267.diff

This patch is on top of bug 1098965, currently UNCO.  It's not quite ready for checkin because I believe it exposes a bug somewhere in the native Promises code.

With the patch, toolkit/modules/tests/xpcshell/test_sqlite.js times out on test_close_database_on_gc().

I can't see anything in particular that I did to cause that timeout.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Right now, we are relying upon Promise.jsm's better error reporting. Once we have landed bug 1083361 and its followups, we can stop relying upon it. Let's wait until then for this bug.

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: bug1095267.diff

Updated for PromiseUtils.  Still hangs with Promise.all for test_close_database_on_gc.

Comment 4

[Alex Vincent [:WeirdAl]]

In test_close_database_on_gc, adding Components.utils.forceCC(); just before the forceGC call fixes the test.

Also (before the cycle collection) with additional debugging code I saw strong evidence that no database's close() promise was ever resolved in the testcase.

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: bug1095267.diff

Marco apparently beat me to that fix for test_sqlite.js.  Patch updated to tip.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8536109 [details] [diff] [review]
bug1095267.diff

Review of attachment 8536109 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good, but I'd like to wait until we have ported the Promise error mechanism (bug 1083361 & followup) before we land it.

Comment 7

[Alex Vincent [:WeirdAl]]

Are we ready to proceed with this?  The Promise error mechanism bug seems to be resolved.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

I have just filed the blockers. Now to find time to work on them...

Comment 9

[Marcos Caceres [:marcosc]]

A cheap workaround is with minimal re-engineering:

```
function TaskImpl(iterator) {
  if (gMaintainStack) {
    this._stack = (new Error()).stack;
  }
  let resolver;
  let rejecter;
  let promise = new Promise( (resolve,reject) => {
    resolver = resolve;
    rejecter = reject;
  });
  let internals = {
    get resolve(){
      return resolver;
    },
    get reject(){
      return rejecter;
    },
    get promise(){
      return promise;
    },
  }
  Object.defineProperty(this, "deferred", {
    get: () => internals
  });
  this._iterator = iterator;
  this._isStarGenerator = !("send" in iterator);
  this._run(true);
}

```

Looking at ./toolkit/modules/PromiseUtils.jsm, basically does the same.

Comment 10

[Alex Vincent [:WeirdAl]]

(In reply to Marcos Caceres [:marcosc] from comment #9)
> Looking at ./toolkit/modules/PromiseUtils.jsm, basically does the same.

My patch converts us to PromiseUtils.jsm, which uses native promises...

Comment 11

[Kris Maglione [:kmag]]

Paolo, is there anything preventing us from migrating Task.jsm to native promises, at this point?

The use of Promise.jsm promises, especially their affects on call stacks, is one of my biggest debugging headaches at this point. I'd be happy to do any work that's needed to make this happen.

Comment 12

[:Paolo Amadini]

(In reply to Kris Maglione [:kmag] from comment #11)
> Paolo, is there anything preventing us from migrating Task.jsm to native
> promises, at this point?

Bug 1156180 appears resolved but is actually a duplicate of bug 1242505, still unresolved. The unhandled rejections reporting for browser-chrome tests is the last thing that needs to be fixed before we can switch Task.jsm to DOM Promise and deprecate Promise.jsm in bug 939636.

> The use of Promise.jsm promises, especially their affects on call stacks, is
> one of my biggest debugging headaches at this point. I'd be happy to do any
> work that's needed to make this happen.

There is also bug 1144353 that is high value and would make Task stacks much easier to follow. That is not a blocker for this bug, though it would make this bug less relevant for stack issues.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kris Maglione [:kmag] from comment #11)
> The use of Promise.jsm promises, especially their affects on call stacks, is
> one of my biggest debugging headaches at this point. I'd be happy to do any
> work that's needed to make this happen.

That's exactly my sentiment and remains on my wishlist-todo (although I hope you beat me to it!). I'm refusing to land a r+'d Sync patch that moves to Task.jsm instead of spinning event loops until I get sane stacks with it applied. 

(In reply to :Paolo Amadini [No reviews] from comment #12)
> Bug 1156180 appears resolved but is actually a duplicate of bug 1242505,
> still unresolved. The unhandled rejections reporting for browser-chrome
> tests is the last thing that needs to be fixed before we can switch Task.jsm
> to DOM Promise and deprecate Promise.jsm in bug 939636.

Is that really a hard dependency? Useless Task.jsm stacks costs me orders of magnitude greater than this facility saves me. I understand that is anecdotal and YMMV, but the pain is real.

Comment 14

[:Paolo Amadini]

(In reply to Mark Hammond [:markh] from comment #13)
> That's exactly my sentiment and remains on my wishlist-todo (although I hope
> you beat me to it!). I'm refusing to land a r+'d Sync patch that moves to
> Task.jsm instead of spinning event loops until I get sane stacks with it
> applied. 

You're probably just looking for bug 1144353, and there is already a work in progress patch there.

This will filter out all Task.jsm frames from the stack even when they are inspected through normal debugging tools, and it works also for Error objects. Note that to use the whole facility you'll need to have async stacks enabled and make a "Task.Debugging.maintainStack = true;" call, the latter can easily be changed to be a preference though.

This is independent from using Promise.jsm or DOM Promise, because all the Promise frames would also be filtered out regardless.

> (In reply to :Paolo Amadini [No reviews] from comment #12)
> > Bug 1156180 appears resolved but is actually a duplicate of bug 1242505,
> > still unresolved. The unhandled rejections reporting for browser-chrome
> > tests is the last thing that needs to be fixed before we can switch Task.jsm
> > to DOM Promise and deprecate Promise.jsm in bug 939636.
> 
> Is that really a hard dependency? Useless Task.jsm stacks costs me orders of
> magnitude greater than this facility saves me. I understand that is
> anecdotal and YMMV, but the pain is real.

To be clear, this dependency is not about the reporting to the Browser Console, it is about test coverage for unhandled rejections. Removing it would be the asynchronous code analogous to making a test succeed when an unhandled exception is thrown in synchronous code. It is a hard dependency because it affects all code that use Task.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Paolo Amadini [No reviews] from comment #14)
> (In reply to Mark Hammond [:markh] from comment #13)
> > That's exactly my sentiment and remains on my wishlist-todo (although I hope
> > you beat me to it!). I'm refusing to land a r+'d Sync patch that moves to
> > Task.jsm instead of spinning event loops until I get sane stacks with it
> > applied. 
> 
> You're probably just looking for bug 1144353, and there is already a work in
> progress patch there.

It looks like that patch still requires gMaintainStack to be flipped, which seems to limit the usefulness to tests. It's important to Sync that stack traces captured in sync logs have readable stack traces. Also, the patch there is over a year old.

I was under the impression that using DOM promises would avoid the need for that patch entirely (comment 12 reinforces that) and also be useful in non-test code - is that not the case?

> This will filter out all Task.jsm frames from the stack even when they are
> inspected through normal debugging tools, and it works also for Error
> objects. Note that to use the whole facility you'll need to have async
> stacks enabled and make a "Task.Debugging.maintainStack = true;" call, the
> latter can easily be changed to be a preference though.

But not a preference we can default to true, right? So IIUC that makes it fairly useless for stacks captured anywhere other than tests.

Note also that filtering out Task.jsm entries would be good, but isn't my concern - my concern is stack traces that *only* reference Task.jsm/Promise.jsm and don't have *any* entries for the code that actually failed (other than the very top of the stack). Here's a recent example from a failed mochitest:

> 19:31:48     INFO -  707 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_syncui.js | ... - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/browser_syncui.js :: testButtonActions :: line 158
> 19:31:48     INFO -  Stack trace:
> 19:31:48     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/browser_syncui.js:testButtonActions:158
> 19:31:48     INFO -      Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
> 19:31:48     INFO -      this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
> 19:31:48     INFO -      Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
> 19:31:48     INFO -      this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
> 19:31:48     INFO -      Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5
> 19:31:48     INFO -      TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:395:7
> 19:31:48     INFO -      TaskImpl_run@resource://gre/modules/Task.jsm:327:13
> 19:31:48     INFO -      TaskImpl@resource://gre/modules/Task.jsm:280:3
> 19:31:48     INFO -      createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
> 19:31:48     INFO -      Task_spawn@resource://gre/modules/Task.jsm:168:12
> 19:31:48     INFO -      TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:388:16
> 19:31:48     INFO -      TaskImpl_run@resource://gre/modules/Task.jsm:327:13
> 19:31:48     INFO -      TaskImpl@resource://gre/modules/Task.jsm:280:3
> 19:31:48     INFO -      createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
> 19:31:48     INFO -      Task_spawn@resource://gre/modules/Task.jsm:168:12
> 19:31:48     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:791:9
> 19:31:48     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:711:7
> 19:31:48     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:741:59

I'm sure you agree that stack is useless for trying to work out how the stop of the stack was actually called.

> > Is that really a hard dependency? Useless Task.jsm stacks costs me orders of
> > magnitude greater than this facility saves me. I understand that is
> > anecdotal and YMMV, but the pain is real.
> 
> To be clear, this dependency is not about the reporting to the Browser
> Console, it is about test coverage for unhandled rejections.

I've seen a number of review comments go past saying that there's no need to use Promise.jsm (ie, to just use DOM promises) - so in many respects that ship has already sailed. Further, the xpcshell test harness already had special support for tasks, so IIUC it's basically impossible for an exception thrown in a task to remain unreported in xpcshell tests.

For code in Firefox (and as you can see above, also code in tests), it's true we will see the "unhandled rejection" messages, but also true that often the stack in such cases is useless. That's not much of a win IMO.

> Removing it
> would be the asynchronous code analogous to making a test succeed when an
> unhandled exception is thrown in synchronous code.

I don't think that's quite the same - in my experience an unhandled rejection tends to mean "you forgot a trailing .catch(Cu.reportError)" rather than "your code is badly broken". Using Task.jsm makes it far more difficult to get that wrong given how promise rejections turn into exceptions.

I understand these unhandled rejection messages should be a priority and are an excellent tool - but not more excellent than relevant stacks.  People avoiding the use of Task.jsm/Promise.jsm due to getting stacks like the above seems the worst possible outcome - they get neither the usefulness of Task.jsm, nor the unhandled rejection warnings.

Comment 16

[:Paolo Amadini]

(In reply to Mark Hammond [:markh] from comment #15)
> > You're probably just looking for bug 1144353, and there is already a work in
> > progress patch there.
> 
> It looks like that patch still requires gMaintainStack to be flipped, which
> seems to limit the usefulness to tests.

As I said, we can change that to be an about:config preference, or you can still flip the variable through the Browser Console, which would take effect during the current session only.

> It's important to Sync that stack
> traces captured in sync logs have readable stack traces.

Note that for DOM Promise to produce a useful stack you still need to have async stacks enabled, which only happens in Nightly and Developer Edition by default, and not on Beta or Release.

So, if your goal is just debugging after an issue occurred, a boolean "about:config" preference controlling what is now gMaintainStack should work. If your goal is having Sync logs captured by default in production to have async stack traces, moving Task.jsm to DOM Promise would not solve your problem.

> Also, the patch there is over a year old.

Unfortunately I don't think we have made any significant changes in Task.jsm in the meantime, so it should be still usable with no logical conflicts...

> I was under the impression that using DOM promises would avoid the need for
> that patch entirely (comment 12 reinforces that) and also be useful in
> non-test code - is that not the case?

I don't think I said that in comment 12?

The patch in bug 1144353 will make async stacks for Task.jsm available in non-test code too, after flipping the preference or setting gMaintainStack.

> > This will filter out all Task.jsm frames from the stack even when they are
> > inspected through normal debugging tools, and it works also for Error
> > objects. Note that to use the whole facility you'll need to have async
> > stacks enabled and make a "Task.Debugging.maintainStack = true;" call, the
> > latter can easily be changed to be a preference though.
> 
> But not a preference we can default to true, right? So IIUC that makes it
> fairly useless for stacks captured anywhere other than tests.

Right, but DOM Promises won't solve this for Beta and Release either, as noted above.

> my concern is stack traces that *only* reference
> Task.jsm/Promise.jsm and don't have *any* entries for the code that actually
> failed (other than the very top of the stack). Here's a recent example from
> a failed mochitest: [...]
> I'm sure you agree that stack is useless for trying to work out how the stop
> of the stack was actually called.

I totally agree! I'm not sure why we get that stack, but it's surely a bug in the rewriting code.

Rather than trying to understand where the bug is in the complex rewriting logic, I'd just go for bug 1144353 which will probably require less time to fix anyways.

> I've seen a number of review comments go past saying that there's no need to
> use Promise.jsm (ie, to just use DOM promises) - so in many respects that
> ship has already sailed.

This is new production code that remains without coverage of unhandled rejections if exercised by browser-chrome tests, but it doesn't reduce the coverage we already have.

The current situation of Promise.jsm is sub-optimal, and the fact that we don't have test coverage on new code is the reason why I think it would be good to have a dedicated strategy for moving these bugs forward, rather than having individual developers tackle them in their little time available in-between other major projects.

> Further, the xpcshell test harness already had
> special support for tasks, so IIUC it's basically impossible for an
> exception thrown in a task to remain unreported in xpcshell tests.

Correct.

> For code in Firefox (and as you can see above, also code in tests), it's
> true we will see the "unhandled rejection" messages, but also true that
> often the stack in such cases is useless. That's not much of a win IMO.

As I said above, this is a bug in the rewriting code...

> > Removing it
> > would be the asynchronous code analogous to making a test succeed when an
> > unhandled exception is thrown in synchronous code.
> 
> I don't think that's quite the same - in my experience an unhandled
> rejection tends to mean "you forgot a trailing .catch(Cu.reportError)"
> rather than "your code is badly broken".

When fixing the missing coverage for DOM Promise in xpcshell tests, I did find at least one actual bug in production code where an error or success condition wasn't propagated properly. In most other cases it was just a missing "catch". That's the value of test coverage, after all... we catch mistakes in the rare cases where we make mistakes! ;-)

> Using Task.jsm makes it far more
> difficult to get that wrong given how promise rejections turn into
> exceptions.

True.

> I understand these unhandled rejection messages should be a priority and are
> an excellent tool - but not more excellent than relevant stacks.  People
> avoiding the use of Task.jsm/Promise.jsm due to getting stacks like the
> above seems the worst possible outcome - they get neither the usefulness of
> Task.jsm, nor the unhandled rejection warnings.

I agree.

Comment 17

[Alex Vincent [:WeirdAl]]

:-)  The last known blocker on this bug has been cleared... I'm curious if (a) there are other blockers that need to be attached to this bug, and (b) if my original patch for this is still good.  (I doubt the latter, but it would be nice...)

Comment 18

[:Paolo Amadini]

(In reply to Alex Vincent [:WeirdAl] from comment #17)
> :-)  The last known blocker on this bug has been cleared...

:-) I'm happy too!

> (a) there are other blockers that need to be attached to this bug

No, that was the last blocker!

> if my original patch for this is still good.

It might, try to rebase and run a tryserver build? Note that bug 1362882 already converted some uses of "defer" to "new Promise()", so you might want to do something similar here, although using PromiseUtils is fine too.

We'll eventually move away from Task.jsm in favor of async functions, but it will still be around for a while for legacy add-on compatibility. I think removing the Promise.jsm dependency is a step in the right direction.

Comment 19

[Alex Vincent [:WeirdAl]]

Someone ping me after June 10, and I can try to update the patch.  I never did get around to getting contributor privileges...

But it's been three years, and I'm extremely busy these days, so if someone wants to grab this before me and update it, please, be my guest!

Comment 20

[Marco Castelluccio [:marco]]

Attachment: Stop using Promise.jsm in Task.jsm

The only related failing tests seems to be devtools/server/tests/unit/test_promises_actor_list_promises.js: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a50fd5d50b548c3bf6113c5d266fc67a756062c&selectedJob=104303442.

The test is checking that promise.promiseState.timeToSettle is a number, which is only true when the state isn't "pending" (see https://hg.mozilla.org/mozilla-central/file/43039280fe464869428f03b047bb7c762784f44b/devtools/server/actors/object.js#l166).
Can we make the assertion at https://hg.mozilla.org/mozilla-central/file/43039280fe464869428f03b047bb7c762784f44b/devtools/server/tests/unit/test_promises_actor_list_promises.js#l50 conditional on the state not being "pending"?

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Check timeToSettle only if the promise's state is not "pending"

See previous comment and attached patch.

Comment 22

[:Paolo Amadini]

Comment on attachment 8874154 [details] [diff] [review]
Stop using Promise.jsm in Task.jsm

Review of attachment 8874154 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but we'll have to land this next cycle after the recently discovered issues with bug 1242505 are solved, so clearing review for now.

::: toolkit/modules/Task.jsm
@@ +273,5 @@
>    }
> +  this.promise = new Promise((resolve, reject) => {
> +      this.resolver = resolve;
> +      this.rejecter = reject;
> +  });

Two space indentation, also call these properties just "_resolve" and "_reject".

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Stop using Promise.jsm in Task.jsm

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa5389ec6b2
Assert the promise's timeToSettle is a number only if its state isn't 'pending'. r=fitzgen

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/baa5389ec6b2

Comment 26

[:Paolo Amadini]

We have to wait a few days to verify that bug 1242505 does not lead to more intermittent failures, then we'll be able to land this patch.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2063c45f91f4
Stop using Promise.jsm in Task.jsm. r=paolo

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2063c45f91f4