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.

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

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.

Sometimes we miss the transaction callback.

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

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 on attachment 8424415 [details] [diff] [review] patch r=me with those things addressed

Blocks a blocker.

Verifyme on 1.4 using steps in 1009746 when landed.

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

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

Thanks :)

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

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