967119 - Hold a wake lock while the contacts DB upgrade is running

Status: RESOLVED FIXED

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

Description

[Reuben Morais [:reuben]]

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).

Comment 1

[Reuben Morais [:reuben]]

Attachment: Hold a CPU lock while the upgrade is running

https://tbpl.mozilla.org/?tree=Try&rev=a5c96a67973b

Comment 2

[Gregor Wagner [:gwagner]]

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.

Comment 3

[Reuben Morais [:reuben]]

Attachment: Hold a CPU lock while the upgrade is running

"Probably not". I then realized I don't even need anything special in IndexedDBHelper.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 5

[Reuben Morais [:reuben]]

(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.

Comment 6

[Reuben Morais [:reuben]]

Attachment: Hold a CPU lock while the upgrade is running, v2

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 8

[Reuben Morais [:reuben]]

Attachment: Hold a CPU lock while the upgrade is running, v3

(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.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 10

[Reuben Morais [:reuben]]

Attachment: Hold a CPU lock while the upgrade is running, v3

Carrying r=bent.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3db93067735a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3db93067735a