Switch Task.jsm to use Promise.jsm

VERIFIED FIXED in Firefox 31

Status

()

Toolkit
Async Tooling
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: gps, Assigned: Paolo)

Tracking

(Blocks: 4 bugs)

Trunk
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox31 fixed)

Details

(Whiteboard: p=8 s=it-31c-30a-29b.2 [qa-])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 768474 [details] [diff] [review]
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
Attachment #768474 - Flags: review?(dteller)
(Reporter)

Updated

4 years ago
Assignee: nobody → gps
(Reporter)

Updated

4 years ago
Version: 22 Branch → Trunk
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+ :)
Attachment #768474 - Flags: review?(dteller) → feedback+
(Reporter)

Comment 3

4 years ago
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
(Reporter)

Comment 4

4 years ago
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.
(Reporter)

Comment 5

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

But, everything else looks green!
(Reporter)

Comment 6

4 years ago
/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.
(Reporter)

Comment 7

4 years ago
I'm not actively working on this. Up for the taking.
Assignee: gps → nobody
Up-to-date try: https://tbpl.mozilla.org/?tree=Try&rev=782f6e3f0cc4
(Linux only)
Component: General → Async Tooling
Whiteboard: [Async]
Blocks: 932266

Updated

3 years ago
Whiteboard: [Async] → [Async:ready]
(Assignee)

Updated

3 years ago
Blocks: 881047
(Assignee)

Updated

3 years ago
Depends on: 973239
(Assignee)

Comment 9

3 years ago
Created attachment 8394759 [details] [diff] [review]
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
Assignee: nobody → paolo.mozmail
Attachment #768474 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8394759 - Flags: review?(mak77)
(Assignee)

Comment 10

3 years ago
Created attachment 8394855 [details] [diff] [review]
Fixed patch

Fixed a few errors in the previous patch.

https://tbpl.mozilla.org/?tree=Try&rev=c3eff16ee4b8
Attachment #8394759 - Attachment is obsolete: true
Attachment #8394759 - Flags: review?(mak77)
Attachment #8394855 - Flags: review?(mak77)
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)
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.
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.

Updated

3 years ago
Flags: needinfo?

Updated

3 years ago
Flags: needinfo? → needinfo?(paolo.mozmail)

Updated

3 years ago
Attachment #8394855 - Flags: review?(mak77)
(Assignee)

Updated

3 years ago
Depends on: 988341
(Assignee)

Comment 14

3 years ago
(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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 15

3 years ago
Created attachment 8397106 [details] [diff] [review]
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.
Attachment #8394855 - Attachment is obsolete: true
Attachment #8397106 - Flags: feedback?(mano)
(Assignee)

Comment 16

3 years ago
Created attachment 8397791 [details] [diff] [review]
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.
Attachment #8397106 - Attachment is obsolete: true
Attachment #8397106 - Flags: feedback?(mano)
Attachment #8397791 - Flags: review?(mano)
Attachment #8397791 - Flags: review?(mak77)
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.
(Assignee)

Comment 18

3 years ago
(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.
(Assignee)

Comment 19

3 years ago
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 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.
Attachment #8397791 - Flags: review?(mano) → review+
(Assignee)

Updated

3 years ago
Depends on: 989984
(Assignee)

Comment 21

3 years ago
(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.
(Assignee)

Comment 22

3 years ago
Created attachment 8399403 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Attachment #8397791 - Flags: review?(mak77)
(Assignee)

Comment 23

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/8b2f6ad94248
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
Also xpcshell crashes on linux64 debug.
https://tbpl.mozilla.org/php/getParsedLog.php?id=37010344&tree=Fx-Team etc

Updated

3 years ago
Blocks: 950073
Whiteboard: [Async:ready] → [Async:ready] p=0
Blocks: 988048
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.
(Assignee)

Updated

3 years ago
Depends on: 991738
(Assignee)

Comment 27

3 years ago
(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.
Blocks: 991797
(Assignee)

Updated

3 years ago
Depends on: 992223

Updated

3 years ago
Blocks: 993314
(Assignee)

Comment 28

3 years ago
Created attachment 8403504 [details] [diff] [review]
The patch
Attachment #8397791 - Attachment is obsolete: true
Attachment #8399403 - Attachment is obsolete: true
Attachment #8403504 - Flags: review+
(Assignee)

Comment 29

3 years ago
This is ready to land when bug 988313 is green on fx-team.
Keywords: checkin-needed
(Assignee)

Comment 30

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/7262fabfd83e
Keywords: checkin-needed

Updated

3 years ago
Blocks: 994712
https://hg.mozilla.org/mozilla-central/rev/7262fabfd83e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Hi Paolo, can you provide a point estimate for this resolved bug.
No longer blocks: 950073
Flags: needinfo?(paolo.mozmail)
Flags: firefox-backlog+
Whiteboard: [Async:ready] p=0 → [Async:ready] p=0 s=it-31c-30a-29b.2 [qa?]
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.
status-firefox31: --- → fixed
Whiteboard: [Async:ready] p=0 s=it-31c-30a-29b.2 [qa?] → [Async:ready] p=0 s=it-31c-30a-29b.2 [qa-]
(Assignee)

Comment 34

3 years ago
(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.
Flags: needinfo?(paolo.mozmail)
Whiteboard: [Async:ready] p=0 s=it-31c-30a-29b.2 [qa-] → p=8 s=it-31c-30a-29b.2 [qa-]

Updated

3 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Updated

3 years ago
Blocks: 996671
(Assignee)

Updated

3 years ago
No longer blocks: 881047
You need to log in before you can comment on or make changes to this bug.