Move SettingsService to use SettingsRequestManager as a backend

RESOLVED FIXED in 2.1 S4 (12sep)

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

unspecified
2.1 S4 (12sep)
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(3 attachments, 4 obsolete attachments)

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.
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.
Attachment #8432911 - Flags: review?(gene.lian)
Attachment #8432911 - Flags: review?(anygregor)
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.
Attachment #8432911 - Flags: review?(gene.lian) → review-
Attachment #8432910 - Attachment is obsolete: true
Attachment #8432910 - Flags: review?(anygregor)
Attachment #8435455 - Flags: review?(anygregor)
Attachment #8435455 - Attachment description: 0006-Bug-1015518-Use-SettingsRequestManager-as-SettingsSe.patch → Patch 1 (v2) - Use SettingsRequestManager as SettingsService Backend
Attachment #8432911 - Attachment is obsolete: true
Attachment #8432911 - Flags: review?(anygregor)
Attachment #8435458 - Flags: review?(anygregor)
Fixed issue with wrong boolean check, removed debug message.
Attachment #8435460 - Flags: review?(gene.lian)
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.
Attachment #8435460 - Flags: review?(gene.lian) → review+
Fixed issue with files that got rebased into the wrong patch
Attachment #8435455 - Attachment is obsolete: true
Attachment #8435455 - Flags: review?(anygregor)
Attachment #8437148 - Flags: review?(anygregor)
Attachment #8435458 - Flags: review?(anygregor) → review?(bent.mozilla)
Attachment #8437148 - Flags: review?(anygregor) → review?(bent.mozilla)
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 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?
Attachment #8435458 - Flags: review?(bent.mozilla)
(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.
(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.
Fixed debug messages to have proper check prefix, as requested.
Attachment #8437148 - Attachment is obsolete: true
Attachment #8472789 - Flags: review?(bent.mozilla)
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.
Attachment #8472789 - Flags: review?(bent.mozilla) → review+
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?
Flags: needinfo?(kyle)
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
Sheriffs - please don't merge this to m-c until comment#21 is addressed, thanks :)
Depends on: 1059766
(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.
(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
No longer depends on: 1059766
https://hg.mozilla.org/mozilla-central/rev/e83b5aa1d6c0
https://hg.mozilla.org/mozilla-central/rev/1068585f6572
https://hg.mozilla.org/mozilla-central/rev/6b1690ad2cf5
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
Component: Gaia::Settings → DOM: Device Interfaces
Product: Firefox OS → Core
You need to log in before you can comment on or make changes to this bug.