Closed Bug 1053966 Opened 9 years ago Closed 9 years ago

[Settings][Gecko] Move the changed settings from aData to aSubject

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Settings
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
2.1 S4 (12sep)

People

(Reporter: mikeh, Assigned: mikeh)

References

Details

Attachments

(1 file, 9 obsolete files)

Move changed settings from aData to aSubject, v1.8 r=bz,qdot
55.63 KB, patch
mikeh
: review+
Details | Diff | Splinter Review 
Bug 1053433 was about refactoring the parsing of JSON settings-changed strings. This is about moving the changed settings from a string in Observe()'s aData argument to an object in aSubject.

Imagine an interface:

interface nsISettingChanged : nsISupports
{
  readonly attribute DOMString key;
  readonly attribute nsIVariant value;
};

See http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIVariant.idl

The handler thus becomes something like:

NS_IMETHODIMP
SomeClass::Observe(nsISupports* aSubject, const char* aTopic,
                   const char16_t* aData)
{
  MOZ_ASSERT(NS_IsMainThread());
  nsCOMPtr<nsISettingChanged> setting = do_QueryInterface(aSubject);
  if (!setting) {
    return NS_OK;
  }

  if (!setting.GetKey().EqualsASCII("setting-of-interest")) {
    return NS_OK;
  }
  if (!setting.GetValue().GetDataType() != VTYPE_DOMSTRING /* e.g. */) {
    return NS_OK;
  }

  nsString value = setting.GetValue().getAsDOMString();

  // handle value

  return NS_OK;
}

Another option could be to subclass nsISettingChanged:

interface nsIDOMStringSettingChanged: nsISettingChanged
{
  readonly attribute DOMString value;
};

And the handler could be:

NS_IMETHODIMP
SomeClass::Observe(nsISupports* aSubject, const char* aTopic,
                   const char16_t* aData)
{
  MOZ_ASSERT(NS_IsMainThread());
  nsCOMPtr<nsIDOMStringSettingChanged> setting = do_QueryInterface(aSubject);
  if (!setting) {
    return NS_OK;
  }

  if (!setting.GetKey().EqualsASCII("setting-of-interest")) {
    return NS_OK;
  }

  nsString value = setting.GetValue().getAsDOMString();

  // handle value

  return NS_OK;
}
Moving the settings-changed data to aSubject greatly simplifies the code.

fabrice: this needs some cleaning up, but was wondering what you think.

And I wonder how good our automated test coverage is. :)
Attachment #8479203 - Flags: feedback?(fabrice)
Comment on attachment 8479203 [details] [diff] [review]
WIP - move changed settings from aData to aSubject, v1

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

That looks great!

::: dom/settings/SettingsChangeNotifier.jsm
@@ -90,5 @@
>            return null;
>          }
>          this.broadcastMessage("Settings:Change:Return:OK",
> -          { key: msg.key, value: msg.value });
> -        Services.obs.notifyObservers(this, kMozSettingsChangedObserverTopic,

why did you move that to SettingsManager.js ?
Attachment #8479203 - Flags: feedback?(fabrice) → feedback+
(In reply to Fabrice Desré [:fabrice] from comment #2)

> why did you move that to SettingsManager.js ?

Because SettingsChangeNotifier.jsm only runs in the parent process, while SettingsManager.js runs everywhere. By moving the notifyObservers() call to the latter, it's possible to notify listeners running strictly in child processes (like the CameraControl stack) as well.

I'll clean up the patch and report--you okay to review it?
Reminder to myself to help mikeh on this once bug 1015518 and everything before it lands.
Flags: needinfo?(kyle)
Ok. Bug 900551 and bug 1015518 landed. So, all that /should/ need to change in this patch is moving the SettingsChangeNotifier.jsm changes to SettingsRequestManager, since SettingsChangeNotifier was folded into that and no longer exists.

Ideally, it'd be nice for the dictionary to have its own webidl bindings file too so we didn't have to include SettingsManager everywhere, but that's just me being overly picky. :)
Flags: needinfo?(kyle)
Comment on attachment 8479980 [details] [diff] [review]
Move changed settings from aData to aSubject, v1.1

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

Looks good so far, just address changes I mentioned in last comment and r? me instead of fabrice on the next version since settings is all my fault now.
Attachment #8479980 - Flags: review?(fabrice) → feedback+
Un-bitrot patch due to changes in setting manager, incorporate review feedback.

try-server push:
https://tbpl.mozilla.org/?tree=Try&rev=02326d38a869
Attachment #8479980 - Attachment is obsolete: true
Attachment #8484328 - Flags: review?(kyle)
Comment on attachment 8484328 [details] [diff] [review]
Move changed settings from aData to aSubject, v1.2

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

r+ with removal of code from SettingsManager.js

::: dom/settings/SettingsManager.js
@@ +274,5 @@
> +          {
> +            key: msg.key,
> +            value: msg.value
> +          },
> +          kMozSettingsChangedObserverTopic, "");

Actually, we don't need this up here. Observation in the child is taken care of by the code below, and is subject to lots of permissions checks. Since I believe you're just worried about code in the parent, what you have in SettingsRequestManager should be fine.
Attachment #8484328 - Flags: review?(kyle) → review+
Remove the child-side notifyObservers() call. It's not needed in this patch (though it will be later--in bug 982377).

try-server (re)push:
https://tbpl.mozilla.org/?tree=Try&rev=63a82f80f417
Attachment #8484328 - Attachment is obsolete: true
Attachment #8484386 - Flags: review+
Fix bustage, carrying r+ forward.

try-server push:
https://tbpl.mozilla.org/?tree=Try&rev=747149a17c14
Attachment #8484386 - Attachment is obsolete: true
Attachment #8484489 - Flags: review+
Comment on attachment 8484489 [details] [diff] [review]
Move changed settings from aData to aSubject, v1.4 r=qdot

Forgot I need a DOM peer to review this. The only DOM change is a new dictionary in dom/webidl/SettingsObserver.webidl.
Attachment #8484489 - Flags: review+ → review?(bzbarsky)
Comment on attachment 8484489 [details] [diff] [review]
Move changed settings from aData to aSubject, v1.4 r=qdot

>+dictionary ObserveSettingChanged {

Maybe call it SettingChangeNotification?

>+++ b/dom/audiochannel/AudioChannelService.cpp
>+    ObserveSettingChanged setting;

This would hopefully turn the tree orange...  but hard to tell, given the ifdef; maybe we don't run the relevant test on the relevant platform.

This needs to be 

  RootedDictionary<ObserveSettingChanged> setting(cx);

>+    if (!setting.mValue.isInt32()) {

This is almost certainly wrong.  The caller has basically no control about whether the value isInt32() or isDouble() but integer-valued; the JS engine will randomly (well, depending on what the JIT sees) decide how it wants to represent integers.  You probably want isNumber() here and then conversion to an int (not via toInt32(), via toNumber()).

>+++ b/dom/bluetooth/BluetoothService.cpp
>+  ObserveSettingChanged setting;

RootedDictionary.

>+++ b/dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
>+  dom::ObserveSettingChanged setting;

RootedDictionary.

>+++ b/dom/bluetooth2/BluetoothService.cpp
>+  ObserveSettingChanged setting;

You know the drill.

>+++ b/dom/bluetooth2/bluedroid/hfp/BluetoothHfpManager.cpp
>+  dom::ObserveSettingChanged setting;

And here.

>+++ b/dom/bluetooth2/bluez/BluetoothHfpManager.cpp
>+  dom::ObserveSettingChanged setting;

And here.

>+++ b/dom/fmradio/FMRadioService.cpp
>+  dom::ObserveSettingChanged setting;

Likewise.

>+++ b/dom/geolocation/nsGeolocation.cpp
>+    ObserveSettingChanged setting;

And here.

>+++ b/dom/system/gonk/AudioManager.cpp
>+    dom::ObserveSettingChanged setting;

And here.

>+    if (!setting.mValue.isInt32()) {

And again, can't rely on this being an int.

>+++ b/dom/system/gonk/AutoMounterSetting.cpp
>+  dom::ObserveSettingChanged setting;

And here.

>+    if (!setting.mValue.isInt32()) {

Again, can't rely on it being an int.

>+++ b/dom/system/gonk/TimeZoneSettingObserver.cpp
>+  dom::ObserveSettingChanged setting;

And here.

We could try avoiding the "any" mess by using dictionaries with different "value" types, except it sounds like you want actual checking, not coercion in some cases and don't know the type until you've seen the name in other cases... :(
Attachment #8484489 - Flags: review?(bzbarsky) → review-
Incorporating review feedback, rooting hazard fixen.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=6964e8bd4e60
Attachment #8484489 - Attachment is obsolete: true
Attachment #8486036 - Flags: review?(bzbarsky)
Comment on attachment 8486036 [details] [diff] [review]
Move changed settings from aData to aSubject, v1.5

>+  JS::Rooted<JS::Value> v(aCx, OBJECT_TO_JSVAL(obj));

Whiler we're here... That should be JS::ObjectValue(*obj), not OBJECT_TO_JSVAL(obj).

r=me
Attachment #8486036 - Flags: review?(bzbarsky) → review+
Incorporate review feedback; carrying r+ forward.

Now with 100% fewer missing files:
https://tbpl.mozilla.org/?tree=Try&rev=d231b1e4e77e
Attachment #8486036 - Attachment is obsolete: true
Attachment #8486489 - Flags: review+
Status: NEW → ASSIGNED
Target Milestone: --- → 2.1 S4 (12sep)
My change causes something to go horribly wrong, causing timeouts in the marionette WebAPI tests.

One issue that seems to stand out is a series of IPC errors that don't occur without this patch:

09-10 20:07:48.520  2316  2316 I Gecko   : IPDL protocol error: Handler for AsyncMessage returned error code
09-10 20:07:48.520  2316  2316 I Gecko   : 
09-10 20:07:48.520  2316  2316 I Gecko   : ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x1C00A2,name=PContent::Msg_AsyncMessage) Processing error: message was deserialized, but the handler returned false (indicating failure)
09-10 20:07:48.520  2316  2316 I Gecko   : 
09-10 20:07:48.520  2316  2328 I Gecko   : [Parent 2316] WARNING: waitpid failed pid:2422 errno:10: file ../../../../../m-c/b2g-inbound/ipc/chromium/src/base/process_util_posix.cc, line 261
09-10 20:07:48.520  2316  2316 I Gecko   : IPDL protocol error: Handler for AsyncMessage returned error code
09-10 20:07:48.520  2316  2316 I Gecko   : 
09-10 20:07:48.520  2316  2316 I Gecko   : ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x1C00A2,name=PContent::Msg_AsyncMessage) Processing error: message was deserialized, but the handler returned false (indicating failure)
09-10 20:07:48.520  2316  2316 I Gecko   : 
09-10 20:07:48.520  2316  2316 I Gecko   : IPDL protocol error: Handler for AsyncMessage returned error code
09-10 20:07:48.520  2316  2316 I Gecko   : 
09-10 20:07:48.520  2316  2316 I Gecko   : ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x1C00A2,name=PContent::Msg_AsyncMessage) Processing error: message was deserialized, but the handler returned false (indicating failure)
09-10 20:07:48.520  2316  2316 I Gecko   : 
09-10 20:07:48.520  2316  2316 I Gecko   : IPDL protocol error: Handler for AsyncMessage returned error code
09-10 20:07:48.520  2316  2316 I Gecko   : 
09-10 20:07:48.520  2316  2316 I Gecko   : ###!!! [Parent][DispatchAsyncMessage] Error: (msgtype=0x1C00A2,name=PContent::Msg_AsyncMessage) Processing error: message was deserialized, but the handler returned false (indicating failure)

The constant, 0x1C00A2, doesn't seem to be specified anywhere in the codebase.

qDot, I don't suppose you know what's going on here, eh?
Flags: needinfo?(kyle)
Further to comment 21, the message that seems to fail is "Settings:Finalize":

(gdb) p aMsg
$6 = (const nsString &) @0xbec9e4dc: {<nsAString_internal> = {mData = 0xadc020d8 u"Settings:Finalize", mLength = 17, mFlags = 5}, <No data fields>}
You're running into bug 1065128 also. Trying to fix this ASAP. And sorry you had to track it down too.
Depends on: 1065128
Flags: needinfo?(kyle)
Retry now that patches for bug 1065128 have landed on b2g-inbound:

https://tbpl.mozilla.org/?tree=Try&rev=1f4c42e77cb9
After much hacking and head-desking, the solution seems to be to call .notifyObservers() with the 'setting' object wrapped thus:

  Services.obs.notifyObservers({ wrappedJSObject: setting }, ...);

And on the receiving end, in JS land:

  observe: function(subject, ...) {
    if ('wrappedJSObject' in subject) {
      subject = subject.wrappedJSObject;
    }
    ...
  };

Without the above wrap-age, the observers get subject = {}.

This gets in the way of one of the purposes of this patch: to simplify 'mozsettings-changed' observers in C++-land. WrappedJSToDictionary() in BindingUtils.h might need to be modified to clean this up. Something like:

  template<class T>
  inline bool
  WrappedJSToDictionary(nsISupports* aObject, T& aDictionary)
  {
    nsCOMPtr<nsIXPConnectWrappedJS> wrappedObj = do_QueryInterface(aObject);
    if (!wrappedObj) {
      return false;
    }

    AutoJSAPI jsapi;
    jsapi.Init();

    JSContext* cx = jsapi.cx();
    JS::Rooted<JSObject*> obj(cx, wrappedObj->GetJSObject());
    if (!obj) {
      return false;
    }

    JSAutoCompartment ac(cx, obj);
    bool found = false;
    JS::Rooted<JS::Value> v(cx, JSVAL_NULL);
    if (JS_HasProperty(cx, obj, "wrappedJSObject", &found) && found) {
      JS_GetProperty(cx, obj, "wrappedJSObject", v);
    }

    v = OBJECT_TO_JSVAL(obj);
    return aDictionary.Init(cx, v);
  }
Flags: needinfo?(bzbarsky)
>  Services.obs.notifyObservers({ wrappedJSObject: setting }, ...);

How about:

  setting.wrappedJSObject = setting;
  Services.obs.notifyObservers(setting, ...);

and then the JS consumers can do:

  if ('wrappedJSObject' in subject) {
    subject = subject.wrappedJSObject;
  }

to deal with the double-wrapping and the C++ won't need the hackery, I believe.
Flags: needinfo?(bzbarsky)
wrappedJSObject FTW.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=b9ac70626ba4
Attachment #8486489 - Attachment is obsolete: true
Attachment #8487629 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/fc551e2f062f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.