Closed Bug 798353 Opened 7 years ago Closed 2 years ago

Reuse settings database lock during boot up

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: timdream, Unassigned)

References

Details

Attachments

(1 file, 4 obsolete files)

SettingsListener in chrome has not been updated along with the same thing in Gaia for a really long time. One of the problem with the current version is that it doesn't use the same lock created from |createLock()| for all settings.

The proposed fix will not attempt to copy the SettingsListener from Gaia since it's introducing bug 793239. Instead, all |SettingsListener.observe| is being gathered, and run at once.

The apparent benefit of the fix will be that it will speed up observable boot up time, as the same effect happened on System app. The possible impact is that since the boot up is being speed up, some of the unforeseen race condition might be triggered.

This does not block release, but I would like to get approval for landing this before release.
Attached patch WIP (obsolete) — Splinter Review
Besides mentioned in comment 0, this patch also removed a lot of "defaults" from the code. We should not save the default at two places in the same m-c codebase.

I would like to test this against B2G Desktop and Otoro before submit for review.
Attached patch Patch proposal (obsolete) — Splinter Review
After looking at the current code further, I realized it's not worthy of the time to factoring out all the |SettingsListener.observe()| calls. So, instead, I upgrade the SettingsListener in Gecko to the one we used in Gaia, which give up following benefits:

- reuse of the lock during first run if |observe()| is called multiple times in the same function loop.
- use mozSettings.addObserver() instead of put all the callbacks in an object.
Attachment #668439 - Attachment is obsolete: true
Attachment #797706 - Flags: review?(fabrice)
Attachment #797706 - Attachment is patch: true
Comment on attachment 797706 [details] [diff] [review]
Patch proposal

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

I'd like to see a new version, and try runnables instead of setTimeout

::: b2g/chrome/content/settings.js
@@ +24,2 @@
>  var SettingsListener = {
> +  /* Timer to remove the lock. */

nit: You use // for single line comments elsewhere.

@@ +27,2 @@
>  
> +  /* lock stores here */

same here

@@ +34,5 @@
> +   * whenever possible.
> +   */
> +
> +  getSettingsLock: function sl_getSettingsLock() {
> +    // Each time there is a getSettingsLock call, we pospone the removal

s/pospone/postpone, and add afull stop at the ned of the comment.

@@ +36,5 @@
> +
> +  getSettingsLock: function sl_getSettingsLock() {
> +    // Each time there is a getSettingsLock call, we pospone the removal
> +    clearTimeout(this._timer);
> +    this._timer = setTimeout((function removeLock() {

nit: no need to name functions anymore.

@@ +48,2 @@
>  
> +    // If there isn't we return one.

// If there isn't we create a new one.

@@ +48,3 @@
>  
> +    // If there isn't we return one.
> +    var settings = window.navigator.mozSettings;

let settigs = .... (here and everywhere else you used var)

@@ +54,5 @@
>  
>    observe: function sl_observe(name, defaultValue, callback) {
>      var settings = window.navigator.mozSettings;
>      if (!settings) {
>        window.setTimeout(function() { callback(defaultValue); });

Usually in chrome code we post a runnable to the main thread (they are not throttled like setTimeout). Can you try that instead?
Attachment #797706 - Flags: review?(fabrice) → feedback+
I realized bug 793239 has been fixed so I should remove all that extra workaround.
Depends on: 793239
Attached patch WIP (obsolete) — Splinter Review
Monday.
Attachment #797706 - Attachment is obsolete: true
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #5)
> Created attachment 798150 [details] [diff] [review]
> WIP
> 
> Monday.

I think I need to verify what bug 793239 really behaves because what's being written in bug 793239 comment 3.
Attached patch Patch v2 (obsolete) — Splinter Review
I decided to keep the try { } cache { } block because bug 793239 still make stale lock throw.

Review comments from v1 addressed.
Attachment #798150 - Attachment is obsolete: true
Attachment #798413 - Flags: review?(fabrice)
Comment on attachment 798413 [details] [diff] [review]
Patch v2

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

r=me

::: b2g/chrome/content/settings.js
@@ +62,5 @@
> +      // a SettingsLock object that is no longer valid.
> +      // Until https://bugzilla.mozilla.org/show_bug.cgi?id=793239
> +      // is fixed, we just catch the resulting exception and try
> +      // again with a fresh lock
> +      console.warn('Stale lock in settings_listener.js.',

nit: s/settings_listener.js/settings.js
Attachment #798413 - Flags: review?(fabrice) → review+
Attached patch Patch for commitSplinter Review
Attachment #798413 - Attachment is obsolete: true
Let me push this to try first.
Keywords: checkin-needed
Looks good :)
Keywords: checkin-needed
Update summary to reflect what is being checked-in.
Summary: Remove SettingsListener from b2g/ and reuse settings lock during boot up → Reuse settings database lock during boot up
backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/93158ab9f33d because this doesn't work. I discovered that working on bug 912898 (we would never shut down adb, etc.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we get this backed out on mozille-central as well?
(In reply to Ben Kelly [:bkelly] from comment #18)
> Can we get this backed out on mozille-central as well?

b2g-inbound is merged into mozilla-central regularly; that merge has just happened:
https://hg.mozilla.org/mozilla-central/rev/93158ab9f33d

If you need something backing out quicker than the next merge (which itself is dependant on having a green push to cut from), then the best bet is to backout directly from mozilla-central itself - and then merge that backout into b2g-inbound (that direction doesn't have to wait for green on the backout on mozilla-central).
Fabrice, could you backout the m-c patch too?
Flags: needinfo?(fabrice)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #20)
> Fabrice, could you backout the m-c patch too?

That was already backed out in https://hg.mozilla.org/mozilla-central/rev/93158ab9f33d
Flags: needinfo?(fabrice)
De-assign myself on this bug since I am no longer actively working on this.

The next assignee should write a new patch and make sure the patch is covered by tests. I don't think we have tests cover this part of the code.
Assignee: timdream → nobody
Status: REOPENED → NEW
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.