The gaia-unit-tests are running on all trunk branches of TBPL, but they're currently hidden. We cannot unhide them until their failure rate is less than 10%, but currently it is greater than that, due to a particular bug which can manifest in a number of tests. The failure is: 18:20:22 INFO - NS_ERROR_UNEXPECTED: Unexpected error (http://communications.gaiamobile.org:8080/shared/js/settings_listener.js:56) 18:26:46 INFO - [Parent 3314] ###!!! ABORT: OOM: file ../../dist/include/nsTHashtable.h, line 173 18:26:46 INFO - [Parent 3314] ###!!! ABORT: OOM: file ../../dist/include/nsTHashtable.h, line 173 This doesn't appear to be the fault of a specific test. I've already disabled two other tests for causing this error, but each time the error just pops up in a different context; it seems more likely a generic problem that will need to be fixed before we can fully benefit from running gaia-unit-tests in TBPL. Vivien, David, Dylan, can one of you find an owner for this?

related bugs: bug 897558, bug 898108

Flagging Dylan to find an owner for this.

Maybe Julien or Kaze are good places to start with this one?

The line that is producing this error is : req = this.getSettingsLock().get(name); which does not say much. I think the error pops up in unrelated tests. For example in bug 898108, the dialer/utils test does not use the SettingsListener object. In both cases, we have that "OOM" error, which, to me, seems to be related to a "not enough memory" problem. This could hide a bigger problem (eg: we possibly leak stuff), but this is too deep in gecko for me. Since Gregor wrote most of the Settings API I'm needinfo him here.

We should land a patch that enables the debug output for the settings API to see what's going on. jgriffin: Does cedar work or should we land it on birch?

Either cedar or birch is fine, the tests run on both. Changes made to cedar won't get merged to m-c, changes to birch will.

I enabled logging for the settings API https://hg.mozilla.org/projects/cedar/rev/4a1b49e1a8e7

Where are those tests in TBPL? https://tbpl.mozilla.org/?tree=Cedar&showall=1&rev=4a1b49e1a8e7 doesn't seem to have them.

I think this is what we are looking for: https://tbpl.mozilla.org/php/getParsedLog.php?id=25823214&tree=Cedar&full=1

Seems like we don't have the settings permission but we still create the navigator object and we can call createLock.

(In reply to Gregor Wagner [:gwagner] from comment #10) > Seems like we don't have the settings permission but we still create the > navigator object and we can call createLock. Ah. Yeah, that's me.

Try returning null in createLock. This will possibly still break Gaia, but let's see if we can land a simple fix for now and then fix properly: https://tbpl.mozilla.org/?tree=Try&rev=1e469bde7505

Let's do a proper fix.

Forgot to qref the CheckPermission call.

I'm trying to understand the desired behavior here. Is the basic goal of the patch to have navigator.mozSettings be null if neither the settings-read or settings-write permission is set? If so, why is that better than not defining the property at all if the permissions are not set?

Also, do we really want window.SettingsManager to exist on non b2g and on b2g when the app is not certified?

I really think unit tests should _not_ depends on API like mozSettings. In unit tests we should replace the actual API (if present) by a object we control. Actually we do this for most tests already (see our mock in [1]) and if some existing unit tests depend on an existing mozSettings they should be fixed. [1] https://github.com/mozilla-b2g/gaia/blob/master/shared/test/unit/mocks/mock_navigator_moz_settings.js But still, what does the "OOM" error mean ? Could such an error happen in usual device usage situation ?

(In reply to Boris Zbarsky (:bz) from comment #16) > I'm trying to understand the desired behavior here. > > Is the basic goal of the patch to have navigator.mozSettings be null if > neither the settings-read or settings-write permission is set? That, and undefined if the pref is not set or if the app is not certified on B2G. > If so, why is that better than not defining the property at all if the > permissions are not set? Because people want to distinguish "this platform does not support mozSettings at all" (navigator.mozSettings is undefined) and "this platform supports mozSettings, but you can't use it" (navigator.mozSettings is null). So far the only use case for this behavior is the Marketplace app, which wants to hide applications or show them as disabled if they depend on APIs that your device doesn't support. Arguably we could find better ways to do this, maybe even with a completely new WebAPI for querying API support, but that proposal hasn't been made. If you review- this patch based on its design, I can come up with a draft for an API support query thing and see where we get.

> Because people want to distinguish "this platform does not support mozSettings at all" > (navigator.mozSettings is undefined) and "this platform supports mozSettings, but you > can't use it" (navigator.mozSettings is null). Hmm... Ok, I seem to recall sicking making that argument. Let's assume we do want that behavior for the moment. Per IRC discussion, I filed bug 900994 to enable us to hide the property declaratively in b2g non-certified apps, and Reuben will handle returning null by adding a special case to the navigator resolve hook. Then we can avoid adding explicit C++ code for the getter.

I pushed these patches to Cedar, let's see if they get the failure rate low enough: https://tbpl.mozilla.org/?tree=Cedar&rev=8e6e1b1e54dd

Using an iframe to get a new global with the permissions set.

Comment on attachment 785150 [details] [diff] [review] Resolve hook check for mozSettings >+ aValue.set(JSVAL_NULL); aValue.setNull(); You still need the other changes to use Func in the IDL and the implementation of the checker function, yes? Also, this needs to move down to after we checked for the prop being enabled at all. Otherwise you'll end up returning null when you want to return undefined.

Comment on attachment 785151 [details] [diff] [review] Some tests for the changes, v2 >+ is(navigator.mozSettings, undefined, "navigator.mozSettings is undefined"); This actually fails to detect the bug in the previous patch because is() uses == and null == undefined. Yes, this is all insane. If we don't have a bug on making is() use ===, please file it! Please use ok(navigator.mozSettings === undefined, "navigator.mozSettings is undefined"); for now. r=me with that.

(In reply to Boris Zbarsky (:bz) from comment #25) > Yes, this is all insane. If we don't have a > bug on making is() use ===, please file it! I'm afraid that would break half of the tree. There's ise(), which uses ===.

(In reply to Boris Zbarsky (:bz) from comment #24) > You still need the other changes to use Func in the IDL and the > implementation of the checker function, yes? We can land that separately.

Comment on attachment 785537 [details] [diff] [review] Resolve hook check for mozSettings, v2 Hmm. So the only important part of the fix was to return null instead of the object if the permission check fails? OK. r=me

Thanks! https://hg.mozilla.org/integration/b2g-inbound/rev/84530975d22f https://hg.mozilla.org/integration/b2g-inbound/rev/be79c6478634

(In reply to Boris Zbarsky (:bz) from comment #28) > Comment on attachment 785537 [details] [diff] [review] > Resolve hook check for mozSettings, v2 > > Hmm. So the only important part of the fix was to return null instead of > the object if the permission check fails? The very important part was fixing the bustage caused by the object being there when the window doesn't have permission. Some people still haven't agreed on undefined for non-certified apps (for mozSettings specifically), so we can land that in the future. (In reply to Gregor Wagner [:gwagner] from comment #29) Thanks for landing!

Backed out for B2G mochitest-4 failures. https://hg.mozilla.org/integration/b2g-inbound/rev/db78acb0cbc3 https://tbpl.mozilla.org/php/getParsedLog.php?id=26167362&tree=B2g-Inbound

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31) > Backed out for B2G mochitest-4 failures. > https://hg.mozilla.org/integration/b2g-inbound/rev/db78acb0cbc3 > > https://tbpl.mozilla.org/php/getParsedLog.php?id=26167362&tree=B2g-Inbound Ugh, I wonder if the way we run mochitests in the emulator means we already have the permission when the test starts. Let's see: https://tbpl.mozilla.org/?tree=Try&rev=b5ed7f0e7548

I pushed again to try: https://tbpl.mozilla.org/?tree=Try&rev=cc367760accc

remote: https://hg.mozilla.org/integration/b2g-inbound/rev/0b1ab27423bc remote: https://hg.mozilla.org/integration/b2g-inbound/rev/f68f3d7f6b86

https://hg.mozilla.org/mozilla-central/rev/0b1ab27423bc https://hg.mozilla.org/mozilla-central/rev/f68f3d7f6b86

Candice Serran deleted the linked story in Pivotal Tracker