852411 - Use the new "Promise.jsm" implementation in Firefox Health Report

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P4)

Description

[Richard Newman [:rnewman]]

Follow-up from hack in Bug 845842.

Comment 1

[:Paolo Amadini]

Now that bug 810490 landed, depending on whether the FHR client code has
dependencies on the helper functions in the Add-on SDK module "promise.js",
it can be switched to use the "Promise.jsm" module directly, instead of blocking
the improvements to "promise.js" in bug 881047.

In either case, the work here consists in figuring out which tests start to fail
because of the new delayed behavior compliant with "Promises/A+". The following
is an example already reported in bug 810490.

Various events occur that cause data to be reported, and tests may wait for the
same events (for instance, a DOM mutation or other DOM event). But FHR writes
data in the background, and we can't know when the operation completes - so,
tests use executeSoon after receiving the event that triggers FHR.

Fortunately, FHR has a write queue internally, thus if we change tests to trigger
a fake write after receiving the event, and wait for it, we know for sure that the
previous asynchronous write has completed and we can get a reliable result.

Comment 2

[Stefan Mirea [:smirea]]

Attachment: Migrating from promise.js to Promise.jsm;

Migrated the services/ from using the old "resource://gre/modules/commonjs/sdk/core/promise.js" to "resource://gre/modules/Promise.jsm". As the API interfaces were mostly the same, for the most part, no
 other changes had to be made other than simply replacing the include path from one to the other.

However, in "services/datareporting/tests/xpcshell/test_policy.js" several tests started to fail due to the new delayed behaviour compliant with "Promises/A+" which caused consequent functions to be evaluated before the Promise was resolved thus resulting in several undefined attribute errors.

To fix this, all methods in the "/services/datareporting/policy.js" file that used promises were modified to also return the promise. As a result, all the tests that failed due to this asynchronicity issue were changed into tasks and all their containing promise function calls were yielded, forcing a synchronus test

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 761807 [details] [diff] [review]
Migrating from promise.js to Promise.jsm;

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

I want to r+ this. However, there are some mochitest browser chrome failures from this change:

browser/base/content/test/browser_urlbar_search_healthreport.js
browser/components/search/test/browser_healthreport.js
browser/base/content/test/browser_datareporting_notification.js

And possibly more. IIRC these tests rely on executeSoon() (wait for next tick in event loop). Since promises are now delayed by another tick, well, yeah.

You can run these tests locally via e.g.:

mach mochitest-browser browser/base/content/test/browser_urlbar_search_healthreport.js

I pushed a try build to https://tbpl.mozilla.org/?tree=Try&rev=ab7bd4a344ae. It should have a comprehensive list of test failures in a few hours.

::: services/datareporting/policy.jsm
@@ +94,5 @@
>     * @param error
>     *        (Error) Explains what went wrong.
>     */
>    onUserNotifyFailed: function onUserNotifyFailed(error) {
> +    this.deferred.reject(error);

I'm kinda surprised we didn't need to return (and yield) the promise here! Perhaps it points to a lack of test coverage? I don't care too much since this code is slated for removal in bug 862563.

Comment 4

[Stefan Mirea [:smirea]]

Attachment: Migrating from promise.js to Promise.jsm;

Fixed the test failures by updating "services/datareporting/policy.jsm" to use Promises.jsm and by removing one redundant test in "browser/base/content/test/browser_datareporting_notification.js"

Comment 5

[Gregory Szorc [:gps]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5dad7bad9013

Comment 6

[Gregory Szorc [:gps]]

Looks like that try still has 1 bc failure :(

I'll look into this for you since you can't reproduce it locally.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 762443 [details] [diff] [review]
Migrating from promise.js to Promise.jsm;

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

Looks good. I was able to reproduce the remaining test failure locally and was able to code up a trivial fix (new executeSoon, sadly) to cause it to pass.

I'll check this in for you (after lunch).

::: browser/base/content/test/browser_datareporting_notification.js
@@ -22,5 @@
>    is(policy.notifyState, policy.STATE_NOTIFY_UNNOTIFIED, "Policy is in unnotified state.");
>  
>    service.healthReporter.onInit().then(function onInit() {
>      is(policy.ensureNotifyResponse(new Date()), false, "User has not responded to policy.");
> -    is(policy.notifyState, policy.STATE_NOTIFY_WAIT, "Policy is waiting for notification response.");

Just so it's documented somewhere, I suggested this verbally to Stefan.

This check is excessive and redundant with existing xpcshell tests. It was put in here for good measure, but it isn't strictly required

Making this work would require exposing promises from the bowels of FHR into mochitests. It's not worth the effort, IMO.

::: browser/base/content/test/browser_urlbar_search_healthreport.js
@@ +59,5 @@
>            is(newCount, oldCount + 1, "Exactly one search has been recorded.");
>            finish();
>          });
> +      })
> +      )

The formatting is off here. We want indentation after every { and }. I'll fix this for you and show you what I did.

::: services/datareporting/policy.jsm
@@ +158,5 @@
>     */
>    onNoDataAvailable: function onNoDataAvailable() {
>      this.state = this.NO_DATA_AVAILABLE;
>      this.promise.resolve(this);
> +    return this.promise.promise;

We should consider renaming this.promise to this.deferred as well, but meh.

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9080f6701733

Comment 9

[Gregory Szorc [:gps]]

And backed out for causing some xpcshell tests to not run test functions. I suspect the cause has to do with add_task() test functions not being registered until a later tick. Will file a new bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/86155a5809da

Comment 10

[Gregory Szorc [:gps]]

Fixed. Turns out run_test() is expected to run synchronously. We had a promise in there before and it worked because it was being resolved on the same tick. Chalk up something else that would be detected if we "fuzzed" when promises are resolved.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b384740198c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b384740198c