1012214 - Settings service doesn't call handle callback created in lock object

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Dave Hylands [:dhylands]]

Attachment: Test I was trying to add

While investigating bug 1009746, I discovered that the restart which was supposed to happen after downloading an update wasn't happening.

This is where the restart is supposed to happen:
http://dxr.mozilla.org/mozilla-central/source/b2g/components/UpdatePrompt.js#378

The callback created on line 381, and registered against the handle attribute (line 397) was never being called.

I was going to add a simple test case to reproduce the problem (which I've attached), but I discovered that the chrome settings tests weren't being run at all.

When I tied to enable the test they failed for a different reason (I'll file a bug to block this one).

This bug is about fixing SettingsService to call the handle callback.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: test

Thats my working test case for the callback passed into createLock.

Comment 2

[Gregor Wagner [:gwagner]]

Comment on attachment 8424404 [details] [diff] [review]
Test I was trying to add

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

::: dom/settings/tests/test_settings_service.js
@@ +92,5 @@
> +        lock3.set("prev_foobar", value, null, null);
> +      }
> +    });
> +    is(getHandleCalled, true, "handle (for get) was called")
> +    is(lockHhandleCalled, true, "handle (for lock) was called");

Hm the callbacks are all async so I don't think this test is correct.

Comment 3

[Gregor Wagner [:gwagner]]

Attachment: patch

Sometimes we miss the transaction callback.

Comment 4

[Dave Hylands [:dhylands]]

I can confirm that with attachment 8424415 [details] [diff] [review] applied then the phone now reboots after applying the update.

Comment 5

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

Comment on attachment 8424415 [details] [diff] [review]
patch

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

::: dom/settings/SettingsService.js
@@ +164,5 @@
>        while (lock = this._settingsService._locks.dequeue()) {
>          if (!lock._transaction) {
>            lock._transaction = lock._settingsService._settingsDB._db.transaction(SETTINGSSTORE_NAME, "readwrite");
> +          if (lock._transactionCallback) {
> +            lock._transaction.oncomplete = lock._transactionCallback.handle;

This still needs the try/catch treatment that the other callbacks get.

@@ +232,5 @@
>    createLock: function createLock(aCallback) {
>      var lock = new SettingsServiceLock(this);
>      this._locks.enqueue(lock);
> +    if (aCallback) {
> +      lock._transactionCallback = aCallback;

Wouldn't it make more sense to pass aCallback into the SettingsServiceLock constructor?

::: dom/settings/tests/test_settings_service_callback.js
@@ +21,5 @@
> +        next();
> +      },
> +
> +      handleError: function(name) {
> +        ok(false, "error: " + name);

You should do next() here otherwise the test will timeout (which will still show up as a failure but it just makes the test harness slower)

Comment 6

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

Comment on attachment 8424415 [details] [diff] [review]
patch

r=me with those things addressed

Comment 7

[Jason Smith [:jsmith]]

Blocks a blocker.

Comment 9

[Tony Chung [:tchung]]

Verifyme on 1.4 using steps in 1009746 when landed.

Comment 10

[Gregor Wagner [:gwagner]]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f98a688c79c7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs a bit of rebasing on b2g30 for v1.4 uplift.

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Needs a bit of rebasing on b2g30 for v1.4 uplift.

This should apply now after taking the 1.4 patch from 999572.
Thanks!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Thanks :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/668ac63b8625

Comment 16

[Tony Chung [:tchung]]

Verified fix when updating from previous build to this one.  Device will automatically restart and update with the latest nightly.

Gaia      6a391274cd436f8f0d1fad2db8c6b4805703259c
Gecko     https://hg.mozilla.org/mozilla-central/rev/cbe4f69c2e9c
BuildID   20140527040202
Version   32.0a1
ro.build.version.incremental=94

Comment 17

[bmorgan]

1.4: After flashing to 5-27 build; system update was downloaded and installed, and reset phone automatically without problem

1.4 Environmental Variables:
Device: Flame 1.4 MOZ
BuildID: 20140528000201
Gaia: cd595be0a8e975559e8938830df5face89bec3e8
Gecko: af180db8a4bf
Version: 30.0
Firmware Version: 10g-2

2.0 Environmental Variables:
Device: Flame 2.0 MOZ
BuildID: 20140527040202
Gaia: 6a391274cd436f8f0d1fad2db8c6b4805703259c
Gecko: cbe4f69c2e9c
Version: 32.0a1
Firmware Version: 10g-2