Closed Bug 967119 Opened 7 years ago Closed 7 years ago

Hold a wake lock while the contacts DB upgrade is running

Categories

(Core Graveyard :: DOM: Contacts, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: reuben, Assigned: reuben)

References

Details

(Keywords: perf, Whiteboard: [c=progress p= s= u=], [systemsfe])

Attachments

(1 file, 4 obsolete files)

Ben suggested doing this in bug 951806, and it's a great idea. The upgrade can take a long time for large databases and letting the phone sleep can cause it to take a ridiculously long time (Ben says he experienced a 30min long upgrade on his phone).
See Also: → 951806
Status: NEW → ASSIGNED
Keywords: perf
Priority: -- → P2
Whiteboard: [c=progress p= s= u=]
Attachment #8369745 - Flags: review?(anygregor)
How did you test this? Does this even work? Isn't that async and we call unlock right away?
I think you have to wait for the transaction to complete.
"Probably not". I then realized I don't even need anything special in IndexedDBHelper.
Attachment #8369745 - Attachment is obsolete: true
Attachment #8369745 - Flags: review?(anygregor)
Attachment #8371044 - Flags: review?(anygregor)
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=], [systemsfe]
Comment on attachment 8371044 [details] [diff] [review]
Hold a CPU lock while the upgrade is running

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

Stealing from gwagner at his request.

::: dom/contacts/fallback/ContactDB.jsm
@@ +767,5 @@
> +    this.cpuLock = Cc["@mozilla.org/power/powermanagerservice;1"]
> +                     .getService(Ci.nsIPowerManagerService)
> +                     .newWakeLock("cpu");
> +
> +    aTransaction.oncomplete = function() {

It's not clear that this will always be safe. IndexedDBHelper could some day set the oncomplete handler for this transaction and then your code would silently replace it. I'd recommend using addEventListener instead.

Also, you're not handling abort here. If the transaction fails you'll never release the cpu lock.

I'd recommend making a common (local) cleanup function and then adding event listeners for "complete" and "abort" types.
Attachment #8371044 - Flags: review?(anygregor) → review-
Target Milestone: --- → 1.4 S1 (14feb)
(In reply to ben turner [:bent] <vacation until 2/14> (use the needinfo? flag!) from comment #4)
> It's not clear that this will always be safe. IndexedDBHelper could some day
> set the oncomplete handler for this transaction and then your code would
> silently replace it. I'd recommend using addEventListener instead.

We should probably make IDBHelper use addEventListener too.

> Also, you're not handling abort here. If the transaction fails you'll never
> release the cpu lock.
> 
> I'd recommend making a common (local) cleanup function and then adding event
> listeners for "complete" and "abort" types.

Do you mean "error"? And probably "blocked" too.
Attachment #8371044 - Attachment is obsolete: true
Attachment #8373008 - Flags: review?(bent.mozilla)
Comment on attachment 8373008 [details] [diff] [review]
Hold a CPU lock while the upgrade is running, v2

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +776,5 @@
> +    }
> +
> +    aTransaction.addEventListener("complete", unlockCPU);
> +    aTransaction.addEventListener("error", unlockCPU);
> +    aTransaction.addEventListener("blocked", unlockCPU);

No, I meant 'abort', not 'error' or 'blocked' :)

You're adding event listeners to a transaction, not to a request. Error events bubble up from requests to the transaction but in general those should be handled by other code. Blocked events are only delivered to open requests, so they'll never be seen by your code here.
Attachment #8373008 - Flags: review?(bent.mozilla) → review-
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #7)
> You're adding event listeners to a transaction, not to a request.

Ah, yes. I was looking at the wrong docs.
Attachment #8373008 - Attachment is obsolete: true
Attachment #8379348 - Flags: review?(bent.mozilla)
Comment on attachment 8379348 [details] [diff] [review]
Hold a CPU lock while the upgrade is running, v3

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +800,5 @@
> +
> +    function unlockCPU() {
> +      if (outer.cpuLock) {
> +        if (DEBUG) debug("unlocking cpu wakelock");
> +        outer.cpuLock.unlock();

You might as well set outer.cpuLock to null here too, right?
Attachment #8379348 - Flags: review?(bent.mozilla) → review+
Carrying r=bent.
Attachment #8379348 - Attachment is obsolete: true
Attachment #8380224 - Flags: review+
Keywords: checkin-needed
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
https://hg.mozilla.org/mozilla-central/rev/3db93067735a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.