793239 - SettingsLock should have a state property to indicate if it is still valid.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[David Flanagan [:djf]]

It would be great if the SettingsLock object had a state property with values "open" and "closed". Or just a closed boolean property.  Or some way to determine if a cached lock object is still usable.

Right now gaia's SettingsListener.js module assumes that it can do a setTimeout(, 0) and the lock will be valid until that callback fires. But there's a race condition.

I think we can work around the gaia bug with exception handling, so this is probably not a blocker.  But would be nice in the API.

Comment 1

[David Flanagan [:djf]]

Currently, when I use a stale settings lock, it looks like the exception that is thrown is just a very large integer.  I suppose the API should also do something more JS friendly in this case.  A DOMError with InvalidState or something like that.

Comment 2

[Andrew Overholt [:overholt]]

Based on comment #0, not a blocker.

Comment 3

[Reuben Morais [:reuben]]

Attachment: Patch

Apparently it is *really* hard to what you asked in comment 1 from JS, so I didn't change the exception that is thrown.

Comment 4

[Gregor Wagner [:gwagner]]

Comment on attachment 702466 [details] [diff] [review]
Patch

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

Great work!

::: dom/settings/tests/test_settings_basics.html
@@ +722,5 @@
> +      }
> +    }
> +    SimpleTest.executeSoon(cb);
> +  },
> +  function () {

Please move the previous test that deletes the database to the end.
You should also check that the lock is open in some other tests. So check that the lock is open when we create it and during a success callback.

Comment 5

[Reuben Morais [:reuben]]

Attachment: Patch, v2

Alright, thanks! Carrying r=gwagner

Comment 6

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89942aa8ae6a

Comment 7

[(no longer active)]

Backed out because of test failures:

http://hg.mozilla.org/integration/mozilla-inbound/rev/9ca4e8812ed1

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=89942aa8ae6a

Comment 8

[Reuben Morais [:reuben]]

(In reply to :Ehsan Akhgari from comment #7)
> Backed out because of test failures

Thanks. I messed up when qref'ing that last set of changes :(
Will upload complete patch in a bit.

Comment 9

[Reuben Morais [:reuben]]

Attachment: Patch, v2, this time with all changes

Comment 10

[(no longer active)]

Can you please run this through the try server?  Thanks!

Comment 11

[Reuben Morais [:reuben]]

The relevant test passes on Try: https://tbpl.mozilla.org/?tree=Try&rev=bcf82dfcb4fc

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e00134f26069

Comment 13

[Ed Morley [:emorley]]

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