887923 - Switch Task.jsm to use Promise.jsm

Status: VERIFIED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Gregory Szorc [:gps]]

Task.jsm is currently using promise.js. We should switch it to use Promise.jsm.

This will change semantics so things happen on subsequent ticks. I fear that the longer Task.jsm is using promise.js, the more places in the code will depend on same-tick resolution of promises. We should nip this in the bud.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Switch Task.jsm from promise.js to Promise.jsm

Trivial change. I'm willing to bet money this breaks tests, however.

https://tbpl.mozilla.org/?tree=Try&rev=1c60f0b704e8

Comment 2

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

Comment on attachment 768474 [details] [diff] [review]
Switch Task.jsm from promise.js to Promise.jsm

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

I'll wait until we know how many things break before I convert this into a r+ :)

Comment 3

[Gregory Szorc [:gps]]

Multiple consistent failures so far:

browser/components/places/tests/unit/test_browserGlue_corrupt.js
browser/components/places/tests/unit/test_browserGlue_restore.js
browser/components/places/tests/unit/test_clearHistory_shutdown.js
browser/components/places/tests/unit/test_leftpane_corruption_handling.js

Comment 4

[Gregory Szorc [:gps]]

Looking at just test_browserGlue_corrupt.js, it appears that a registered bookmarks observer listener (onEndUpdateBatch) is being called differently (either before or after) some other operation in places (I think a db restore of some kind) has completed. According to TBPL and bug 529544, we've had issues with this test before.

Comment 5

[Gregory Szorc [:gps]]

We may have introduced a few Windows 8 mc failures as well. Running a few retries now.

But, everything else looks green!

Comment 6

[Gregory Szorc [:gps]]

/browser/metro/base/tests/mochiperf/browser_msgmgr_01.js
/browser/metro/base/tests/mochiperf/browser_tabs_01.js
/browser/metro/base/tests/mochitest/browser_onscreen_keyboard.html'

All failed in 4 runs. Likely regression. Although, I don't have a Metro build available to confirm. Nor do I have Windows 8 to verify this.

Comment 7

[Gregory Szorc [:gps]]

I'm not actively working on this. Up for the taking.

Comment 8

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

Up-to-date try: https://tbpl.mozilla.org/?tree=Try&rev=782f6e3f0cc4
(Linux only)

Comment 9

[:Paolo Amadini]

Attachment: Updated patch

Fortunately, after the recent Promise conversions there are not a lot of places left where making Task.spawn aynchronous is causing test issues.

The most relevant one is the use of Task in the URL bar. Since all the API there are synchronous, tests (and maybe production code) expect a synchronous results.

Since reviewing the URL bar interface will require a lot of time, and given that the code is also sensitive because related to the page security tests, I think it is better to just revert it to use callbacks for the moment, effectively keeping the exact current timing and behavior.

This will allow us to unblock this Task.jsm conversion that, as pointed out in comment 0, is a top priority. New tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=c470352ce84d

Comment 10

[:Paolo Amadini]

Attachment: Fixed patch

Fixed a few errors in the previous patch.

https://tbpl.mozilla.org/?tree=Try&rev=c3eff16ee4b8

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8394855 [details] [diff] [review]
Fixed patch

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

I didn't look at the browser and urlbar stuff yet, that requires a bit more attention. I have a question though.

::: browser/modules/test/browser_UITour_detach_tab.js
@@ +49,5 @@
>          waitForElementToBeVisible(newWindowHighlight, function checkHighlightIsThere() {
>            gContentAPI.showMenu("appMenu");
> +          // The event is dispatched asynchronosuly, but we don't have a way for
> +          // waiting for the call to be complete.
> +          executeSoon(() => {

for these, couldn't you PanelUI.panel.addEventListener("popupshown"?

(it would be nice to change UITourTest to run these functions in a Task, so that here we could just yield promisePopupShown() and avoid reindenting everything)

Comment 12

[Marco Bonardo [:mak]]

I'd also prefer if you could split the UITour and bookmarksJSONUtils changes to a separate bug, those are more trivial and could be landed earlier.

Comment 13

[Marco Bonardo [:mak]]

IIRC Mano did the conversion, so I'd be more comfortable if he'd do the review for the getShortcutOrURI part, he may know whether we can do a more direct conversion without going back to callbacks.

Comment 14

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #12)
> I'd also prefer if you could split the UITour and bookmarksJSONUtils changes
> to a separate bug, those are more trivial and could be landed earlier.

Filed bug 988341.

Comment 15

[:Paolo Amadini]

Attachment: Patch including the URL bar code conversion to callback

Mano, what do you think of the approach here? As pointed out in comment 9, if Task.jsm becomes asynchronous then many assumptions on timing will break, and using a system based on synchronous callbacks can unblock the high-priority conversion.

Comment 16

[:Paolo Amadini]

Attachment: The patch

Marco or Mano, do you think you can prioritize this review? This is a _huge_ win for debuggability of Tasks, regression test resilience and the overall Promises deprecation work, and I'd be happy if we can land this now while it's green, since it is a tree-wide change.

https://tbpl.mozilla.org/?tree=Try&rev=c8d18a501f2a

I think I can file a follow-up bug if you think more cleanup is needed here, but unfortunately for many DOM events (especially drag and drop) we can't just use Promises or Tasks because of their asynchronous nature, at least until the DOM introduces support for ES6 promises in events, at which point we'll have many more possibilities.

Comment 17

[Marco Bonardo [:mak]]

I sent an e-mail to Mano to check his availability, if we don't hear back before tuesday, I'll just do this (that means probably a blanket r+). My fear is mostly that Mano wanted this to use promises/task for integrating with future work, and I'd not like to broke that assumption.

Comment 18

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #17)
> My fear is mostly that Mano wanted this to use promises/task for integrating
> with future work, and I'd not like to broke that assumption.

This sounds reasonable, and if Promise-based versions of the same functions are required, we can keep them available by wrapping the callback-based ones, but the Promise-based ones will not be usable here without a significant amount of work. Currently, they're only working because they rely on what is now considered a bug of Add-on SDK Promises (and thus Tasks).

Any solution involving using Promises in the right way here might realistically take weeks, not days, to design, implement, and review for security (including tests adjustments), and this might make this change race with other Promise changes we're doing, or other people introducing code that relies on this bug of Tasks (especially now that Task.async has been advertised and will become popular).

I've personally started working on some apparently simple devtools conversions three weeks ago, with just one or two test failures, and they're still open.

I'd rather not stop the momentum we got and do this change as soon as possible, handling concerns in follow-ups.

Comment 19

[:Paolo Amadini]

Also, the landing of bug 988122 is putting even more pressure on this. It introduces the general availability of Promise objects with a "catch" method, and this forces us to do the same now for Promise.jsm in bug 941920.

The fact that "Task.spawn(...).catch(...)" will not work as expected will definitely be a source of confusion if not addressed.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 8397791 [details] [diff] [review]
The patch

Well, of course this doesn't make me happy. But what does?

So let's see I get this right. The current setup (i.e. w/o this patch) has hidden the fact that this code hasn't been actually async - i.e. tests weren't falling because the cause code did, in fact, run synchronously?

If so, I'm fine with this change after it's validated that there aren't too many addons out there that rely on the promise-variant of this method (I don't expect any, but please double check).

r=mano with all that in mind. Please ask for review again if addons turns out to be a problem.

Comment 21

[:Paolo Amadini]

(In reply to Mano from comment #20)
> So let's see I get this right. The current setup (i.e. w/o this patch) has
> hidden the fact that this code hasn't been actually async - i.e. tests
> weren't falling because the cause code did, in fact, run synchronously?

Yes, that is exactly the issue.

> If so, I'm fine with this change after it's validated that there aren't too
> many addons out there that rely on the promise-variant of this method (I
> don't expect any, but please double check).

There are only a dozen of them listed. Curiously, one is spinning an event loop to simulate synchronous behavior :-)

> r=mano with all that in mind. Please ask for review again if addons turns
> out to be a problem.

I moved the patch from here to bug 989984, and I've filed bug 989990 to ask if we're concerned with those few instances, in which case I can provide a follow-up patch for review.

Comment 22

[:Paolo Amadini]

Attachment: Switch Task.jsm

There were various intermittent browser-chrome failures in the tryserver build:

https://tbpl.mozilla.org/?tree=Try&rev=c8d18a501f2a

Since I know that browser-chrome timeouts were a tree-wide concern at the time, I'll go ahead with this change on the assumption that they are unrelated. I've separated the dependencies so that they can stick in case this is incorrect and this change needs to be backed out.

Comment 23

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/8b2f6ad94248

Comment 24

[Ed Morley [:emorley]]

Backed out for failures on Windows browser-chrome:
https://tbpl.mozilla.org/?tree=Fx-Team&jobname=Win.*browser-chrome&fromchange=9be5c94051be&tochange=8b2f6ad94248

remote:   https://hg.mozilla.org/integration/fx-team/rev/ada13495871f

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Also xpcshell crashes on linux64 debug.
https://tbpl.mozilla.org/php/getParsedLog.php?id=37010344&tree=Fx-Team etc

Comment 26

[Marco Bonardo [:mak]]

the problem in test_markpageas.js is very likely the mixture of add_task and do_test_pending, the test should use a promise to wait for the observer.

Comment 27

[:Paolo Amadini]

(In reply to Marco Bonardo [:mak] from comment #26)
> the problem in test_markpageas.js is very likely the mixture of add_task and
> do_test_pending, the test should use a promise to wait for the observer.

Thanks for the tip, I've cleaned up the test and filed bug 991738.

Comment 28

[:Paolo Amadini]

Attachment: The patch

Comment 29

[:Paolo Amadini]

This is ready to land when bug 988313 is green on fx-team.

Comment 30

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/7262fabfd83e

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7262fabfd83e

Comment 32

[Marco Mucci [:MarcoM]]

Hi Paolo, can you provide a point estimate for this resolved bug.

Comment 33

[u279076]

I don't think this needs QA testing. If this is opening us up to regression in any area, please advise so I can assign it for testing.

Comment 34

[:Paolo Amadini]

(In reply to Marco Mucci [:MarcoM] from comment #32)
> Hi Paolo, can you provide a point estimate for this resolved bug.

I would have originally made a guess for p=8, which turned out to be more or less the case.

This is part of a category of bugs where it's difficult to estimate the effort in advance, as it involves fixing tests across many areas of the source code.