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

RESOLVED FIXED in Firefox 24

Status

Firefox Health Report
Client: Desktop
P4
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: rnewman, Assigned: smirea)

Tracking

unspecified
Firefox 24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Follow-up from hack in Bug 845842.
Depends on: 845842
Blocks: 856878

Updated

4 years ago
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report

Updated

4 years ago
Priority: -- → P4

Comment 1

4 years ago
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.
Blocks: 881047
Summary: Use a better (and fully async) promise implementation → Use the new "Promise.jsm" implementation in Firefox Health Report
(Assignee)

Updated

4 years ago
Assignee: nobody → steven.mirea
(Assignee)

Comment 2

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

Comment 3

4 years ago
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.
Attachment #761807 - Flags: review?(gps) → feedback+
(Assignee)

Comment 4

4 years ago
Created attachment 762443 [details] [diff] [review]
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"
Attachment #762443 - Flags: review?(gps)
(Assignee)

Updated

4 years ago
Attachment #761807 - Attachment is obsolete: true

Comment 5

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

Comment 6

4 years ago
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

4 years ago
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.
Attachment #762443 - Flags: review?(gps) → review+

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9080f6701733
Status: NEW → ASSIGNED

Comment 9

4 years ago
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
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
https://hg.mozilla.org/mozilla-central/rev/2b384740198c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24

Updated

3 years ago
Blocks: 996671

Updated

3 years ago
No longer blocks: 856878

Updated

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