Closed Bug 852411 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: rnewman, Assigned: smirea)

References

Details

Attachments

(1 file, 1 obsolete file)

Follow-up from hack in Bug 845842.
Depends on: 845842
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
Priority: -- → P4
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: nobody → steven.mirea
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 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+
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)
Attachment #761807 - Attachment is obsolete: true
Looks like that try still has 1 bc failure :(

I'll look into this for you since you can't reproduce it locally.
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Blocks: 996671
No longer blocks: 856878
No longer blocks: 881047
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.