1003650 - DEBUG flag in ril_worker is not dynamic changeable by pref "ril.debugging.enabled"

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

In ril_worker, DEBUG flag could only be set during initialization [1]. Thus, after that, the value is fixed and could not be altered by pref value unless you also restart the b2g.

In TelephonyProvider.js, we observe the pref value changed and update the debug flag immediately [2]. IMO, it's a better way for debugging. So I would like to mimic the same mechanism in ril_worker.

[1] https://github.com/mozilla/gecko-dev/blob/ae6c78e6e8134aa74bdf7b697137bba8ee2116e9/dom/system/gonk/ril_worker.js#L14703

[2] https://github.com/mozilla/gecko-dev/blob/ae6c78e6e8134aa74bdf7b697137bba8ee2116e9/dom/telephony/gonk/TelephonyProvider.js#L907

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

We should also apply the change on RadioInterfaceLayer.js.

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Dynamically update debug flag

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8425219 [details] [diff] [review]
Dynamically update debug flag

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1535,5 @@
>    },
>  };
>  
>  function RadioInterfaceLayer() {
> +  this.workerMessenger = new WorkerMessenger();

I don't think we'd like to let RadioInterfaceLayer hold a reference to workerMessenger because RadioInterfaceLayer isn't communicating with worker per the whole structure. I'd suggest letting WokerMessenger itself take care of observing preference change.

@@ +1548,4 @@
>    }
>  
>    Services.obs.addObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
> +  Services.prefs.addObserver(kPrefRilDebuggingEnabled, this, false);

Oh... I just noticed that our current code doesn't pref.removeObserver properly... I'll file another follow-up to fix that.

But please removeObserver properly when shutdown in the patch.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #3)
> Comment on attachment 8425219 [details] [diff] [review]
> Dynamically update debug flag
> 
> Review of attachment 8425219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1535,5 @@
> >    },
> >  };
> >  
> >  function RadioInterfaceLayer() {
> > +  this.workerMessenger = new WorkerMessenger();
> 
> I don't think we'd like to let RadioInterfaceLayer hold a reference to
> workerMessenger because RadioInterfaceLayer isn't communicating with worker
> per the whole structure. I'd suggest letting WokerMessenger itself take care
> of observing preference change.
> 
> @@ +1548,4 @@
> >    }
> >  
> >    Services.obs.addObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
> > +  Services.prefs.addObserver(kPrefRilDebuggingEnabled, this, false);
> 
> Oh... I just noticed that our current code doesn't pref.removeObserver
> properly... I'll file another follow-up to fix that.
> 
> But please removeObserver properly when shutdown in the patch.

Sorry, please ignore this. See https://bugzilla.mozilla.org/show_bug.cgi?id=926302#c21 for explanation.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: #2 Dynamically update debug flag

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8425367 [details] [diff] [review]
#2 Dynamically update debug flag

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

Looks good, thank you!

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Dynamically update debug flag. r=hsinyi

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=5346b5f559e1

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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