1015518 - Move SettingsService to use SettingsRequestManager as a backend

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

After bug 900551, all transactions on the settings DB happen in the parent. SettingsService should move to using SettingsRequestManager instead of being a reimplementation of what used to live in the child process.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Change SettingsService backend to SettingsRequestManager

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Update users of settings observer "message" field to use boolean

In bug 785298, a string field was introduced to the set request of the SettingsService in order to let observers know where settings came from. This only gets used to check when a setting has happened via the SettingsService. I changed this to be a boolean and handled at lock scope, which makes life MUCH easier when converting the settings service to use the new settings request manager from bug 900551.

The only places I could find that used the message argument were RIL and Wifi, so I've updated those accordingly.

Comment 3

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8432911 [details] [diff] [review]
Patch 2 (v1) - Update users of settings observer "message" field to use boolean

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

::: dom/settings/tests/test_settings_service.js
@@ +92,4 @@
>        }
>  
>        data = JSON.parse(data);
> +      ok(true, JSON.stringify(data));

Just a nit: why not just printing this before parsing, which seems to be the same effect to me.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3364,4 @@
>      // "time.timezone.automatic-update.available" except for the chrome
>      // process.
>      if (aName === kSettingsTimezoneAutoUpdateAvailable &&
> +        aIsInternalSetting) {

Why is this not |!aIsInternalSetting|?

This is the main reason for giving review- for now although it's easy to fix.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Use SettingsRequestManager as SettingsService Backend

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Update SettingsService mochitests

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v2) - Update users of settings observer "message" field to use boolean

Fixed issue with wrong boolean check, removed debug message.

Comment 7

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8435460 [details] [diff] [review]
Patch 3 (v2) - Update users of settings observer "message" field to use boolean

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

Yeap, I assume the isInternalChange can reflect the setting's change is done by the internal Gecko or the Gaia's apps.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Use SettingsRequestManager as SettingsService Backend

Fixed issue with files that got rebased into the wrong patch

Comment 9

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

Comment on attachment 8437148 [details] [diff] [review]
Patch 1 (v3) - Use SettingsRequestManager as SettingsService Backend

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

A lot of this looks duplicated from the other stuff still... Is there no way to merge this better?

::: dom/settings/SettingsService.js
@@ +5,5 @@
>  "use strict"
>  
>  /* static functions */
> +const DEBUG = false;
> +function debug(s) {

Same concerns here as in the other bug about needing to guard all calls to debug() with |if (DEBUG) debug(...);|

@@ +49,5 @@
> +  this._requests = {};
> +  let closeHelper = function() {
> +    debug("closing lock " + this._id);
> +    this._open = false;
> +    this.runOrFinalizeQueries();

I don't see why this is needed here, something else should have called this already right?

Comment 10

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

Comment on attachment 8435458 [details] [diff] [review]
Patch 2 (v2) - Update SettingsService mochitests

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

::: dom/settings/tests/test_settings_service.js
@@ +4,5 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  
> +if (SpecialPowers.isMainProcess()) {
> +  SpecialPowers.Cu.import("resource://gre/modules/SettingsRequestManager.jsm");

Why is this needed?

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #9)
> A lot of this looks duplicated from the other stuff still... Is there no way
> to merge this better?

Yeah, there's some repetition here. The problem is, the SettingsManager has a bunch of DOM stuff in it, while the SettingsService is all callbacks since it's used in chrome. I could probably subclass them both from the same basic functionality, and if you want to make that a seperate bug, that's fine. But I'd rather get this in now and then cleaned up, because combining those is going to be a lot of work, and this is still far better than it was before.

> 
> @@ +49,5 @@
> > +  this._requests = {};
> > +  let closeHelper = function() {
> > +    debug("closing lock " + this._id);
> > +    this._open = false;
> > +    this.runOrFinalizeQueries();
> 
> I don't see why this is needed here, something else should have called this
> already right?

This is here for the same reason it is over in SettingsManager. It's how we start running queries once we're out of lock scope and back into the event loop.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #10)
> Comment on attachment 8435458 [details] [diff] [review]
> Patch 2 (v2) - Update SettingsService mochitests
> 
> Review of attachment 8435458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/settings/tests/test_settings_service.js
> @@ +4,5 @@
> >  const Cc = Components.classes;
> >  const Ci = Components.interfaces;
> >  
> > +if (SpecialPowers.isMainProcess()) {
> > +  SpecialPowers.Cu.import("resource://gre/modules/SettingsRequestManager.jsm");
> 
> Why is this needed?

Same reason it is in the tests in bug 900551. We run these on all platforms, but we only bring in SEttingsRequestManager as part of the b2g shell. Bug 1012403 is supposed to figure out what platforms we want to do this on, but for now, this is what makes things not burn tbpl.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4) - Use SettingsRequestManager as SettingsService Backend

Fixed debug messages to have proper check prefix, as requested.

Comment 14

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

Comment on attachment 8472789 [details] [diff] [review]
Patch 1 (v4) - Use SettingsRequestManager as SettingsService Backend

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

This looks good!

::: dom/settings/SettingsService.js
@@ +30,4 @@
>  const SETTINGSSERVICELOCK_CID        = Components.ID("{d7a395a0-e292-11e1-834e-1761d57f5f99}");
>  const nsISettingsServiceLock         = Ci.nsISettingsServiceLock;
>  
> +function SettingsServiceRequest(aCallback, aName, aValue) {

Nit: I think you should call this 'makeSettingsServiceRequest' or 'createSettingsServiceRequest' or something. Functions that start with capital letters usually mean that they're objects that you're supposed to call 'new' on.

@@ +82,5 @@
> +    let msg = aMessage.data;
> +    // SettingsRequestManager broadcasts changes to all locks in the child. If
> +    // our lock isn't being addressed, just return.
> +    // TODO: Is this safe? Should we set up IPDL channel per lock?
> +    if(msg.lockID != this._id) return;

Nit: One statement per line, space between |if(|

@@ +114,5 @@
> +        this._open = true;
> +        let settings_names = Object.keys(msg.settings);
> +        if (settings_names.length > 0) {
> +          if (settings_names.length > 1) {
> +            if (DEBUG) if (DEBUG) debug("Warning: overloaded setting:" + name);

You probably want this to be:

  if (DEBUG && settings_names.length > 1) {
    debug("Warning: overloaded setting:" + name);
  }

But what is |name| here? I don't think you've defined it yet so this will be an exception.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/9ac939654549
https://hg.mozilla.org/integration/b2g-inbound/rev/04a531d5647e
https://hg.mozilla.org/integration/b2g-inbound/rev/59eb7edaa6ed

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This, bug 900551, and bug 846200 were all backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/c4c490f3b301 for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=46297008&tree=B2g-Inbound

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Reverted https://hg.mozilla.org/integration/b2g-inbound/rev/c4c490f3b301. Build break in https://tbpl.mozilla.org/php/getParsedLog.php?id=46297008&tree=B2g-Inbound was due to bug 1055898, no changes made to original commits.

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

And now backed out again due to marionette webapi test bustage on ics emulator

https://tbpl.mozilla.org/?tree=B2g-Inbound&showall=1&rev=82bed38ff50a

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

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/663b73ba69ec
https://hg.mozilla.org/integration/b2g-inbound/rev/d85b4e48b3b4
https://hg.mozilla.org/integration/b2g-inbound/rev/52b00288469c

Comment 21

[Zac C (:zac)]

Kyle, this patch or something else in thus bunch of Settings patches is causing some very frequent intermittents on b2g-i device testing (Flame).

We get this in the logcat
E/GeckoConsole( 1587): [JavaScript Error: "ReadOnlyError: A mutation operation was attempted in a READ_ONLY transaction." {file: "resource://gre/modules/SettingsRequestManager.jsm" line: 499}]

It occurs just after we've started up the device for the test run so manipulating some settings.
The javascript we're executing that is causing the above error message is:
https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_data_layer.js#L207

It fails to finish and the test times out. It's intermittent though, sometimes it works, sometimes it does not.

Have you any idea why we're encountering it?

Comment 22

[Zac C (:zac)]

It is also causing similar (different stack but same root cause) intermittents on TBPL:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46934177&tree=B2g-Inbound#error0

Comment 23

[Zac C (:zac)]

Sheriffs - please don't merge this to m-c until comment#21 is addressed, thanks :)

Comment 24

[Jason Smith [:jsmith]]

(In reply to Zac C (:zac) from comment #22)
> It is also causing similar (different stack but same root cause)
> intermittents on TBPL:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46934177&tree=B2g-
> Inbound#error0

This might also be why we're seeing bug 1059766. I think we should back this out before merging b2g-inbound.

Comment 25

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #24)
> (In reply to Zac C (:zac) from comment #22)
> > It is also causing similar (different stack but same root cause)
> > intermittents on TBPL:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=46934177&tree=B2g-
> > Inbound#error0
> 
> This might also be why we're seeing bug 1059766. I think we should back this
> out before merging b2g-inbound.

Looks like Ryan already backed this out actually:

https://hg.mozilla.org/mozilla-central/rev/7c97c5f7a05e

Comment 26

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://hg.mozilla.org/integration/b2g-inbound/rev/e83b5aa1d6c0
https://hg.mozilla.org/integration/b2g-inbound/rev/1068585f6572
https://hg.mozilla.org/integration/b2g-inbound/rev/6b1690ad2cf5

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e83b5aa1d6c0
https://hg.mozilla.org/mozilla-central/rev/1068585f6572
https://hg.mozilla.org/mozilla-central/rev/6b1690ad2cf5