Closed Bug 1030266 Opened 7 years ago Closed 7 years ago

PlacesDBUtils.telemetryHook may not fire callback in case of error


(Toolkit :: Places, defect)

Not set





(Reporter: gps, Assigned: Yoric)




(1 file, 3 obsolete files)

From bug 944873 comment #41

> Looks like the issue may be in PlacesDBUtils.telemetry callback, we may not
> forward the error properly there. That's very likely cause PlacesDBUtils is
> very old code not using Tasks/promises and some errors are indeed swallowed
> (just reportError-ed)
> More specifically the mistake looks being here:
> PlacesDBUtils.jsm#1013
> we call reportResult only in case of success, and reportResult is the one
> that will invoke the callback.
> This code must be changed to callback in any case.

This issue causes FHR to hang, which can lead to a shutdown crash.
Assignee: nobody → dteller
Summary of changes:
1. moved `reportValues` to Promise-style;
2. the health report callback is now called even if something throws an error or if the request fails;
3. fixed an unbound variable (`param`).

Note that 1. was the simplest way I found to ensure 2.

Attachment #8452229 - Flags: review?(mak77)
mak, I would be grateful if you could prioritize this review, as we need to be able to uplift to Aurora before the train leaves the station.
Comment on attachment 8452229 [details] [diff] [review]
Ensuring that the callback is always called

Review of attachment 8452229 [details] [diff] [review]:

Awww, how much I'd like PlacesDBUtils to be rewritten using Task and promiseDBConnection :/ sounds like we'll get a bug filed to do that.

Btw, doesn't look like this is handling the rejection, Promise.all rejects if any promise rejects, but the callback is again only invoked on success

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +984,4 @@
> +        try {
> +          stmt.executeAsync({
> +            handleError: reject,

doesn't look like this rejection gets reported anywhere, it would be nice to at least have a reportError for it...

@@ +1018,5 @@
> +    if (aHealthReportCallback) {
> +      Promise.all(outstandingProbes).then(() =>
> +        aHealthReportCallback(probeValues)
> +      );

shouldn't you handle the rejection here?
Maybe I'm misreading but the first then doesn't handle it, and promise.all will call the rejection callback if any promise is rejected, that is basically this case. So this doesn't seem to fix this bug.

@@ +1020,5 @@
> +      Promise.all(outstandingProbes).then(() =>
> +        aHealthReportCallback(probeValues)
> +      );
> +    }
> +    

trailing spaces

@@ +1026,1 @@
>      PlacesDBUtils._executeTasks(tasks);

this is wrong, since it's proceeding with the next task without waiting for this one to be complete... but it was wrong already... let's ignore it for now and we'll take care when we rewrite this module in a modern way.
Attachment #8452229 - Flags: review?(mak77) → review-
Oops, embarrassing, I obviously forgot to qref.
My apologies, I haven't really slept since Friday.

Also, I don't think we want to wait for a complete rewrite, as this error is strongly suspected to be the cause for the crashes of bug 1035290.
Attachment #8452229 - Attachment is obsolete: true
Attachment #8452363 - Flags: review?(mak77)
Comment on attachment 8452363 [details] [diff] [review]
Ensuring that the callback is always called, v2

Wait, what? Something's wrong with my uploads.
Attachment #8452363 - Flags: review?(mak77)
Comment on attachment 8452367 [details] [diff] [review]
Ensuring that the callback is always called, v3

Note for future self: `hg qref` is good. Checking that you're in the right position on the queue is useful, too.
Attachment #8452367 - Flags: review?(mak77)
(In reply to David Rajchenbach Teller [:Yoric] (currently fighting fires, expected return on Tuesday July 8th) from comment #4)
> Also, I don't think we want to wait for a complete rewrite, as this error is
> strongly suspected to be the cause for the crashes of bug 1035290.

I meant to wait only for the last row that was ALREADY wrong before your patch :)
Comment on attachment 8452367 [details] [diff] [review]
Ensuring that the callback is always called, v3

Review of attachment 8452367 [details] [diff] [review]:

yes, this one is better ;)

::: toolkit/components/places/PlacesDBUtils.jsm
@@ +968,5 @@
>        if (!isTelemetry && !probe.healthreport) {
>          continue;
>        }
> +      let promise = new Promise((resolve, reject) => {

last thing I forgot to comment about even if I already noticed it.
Per my personal mental sanity, please don't introduce a lowercase promise variable, anything like probePromise or even just "p" would work, but I don't think it's very sanity-proof to have Promise and promise mixed up in the same code :)
Attachment #8452367 - Flags: review?(mak77) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.