Closed Bug 1193469 Opened 6 years ago Closed 6 years ago

Make mozSettings more defensive on addObserver

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
FxOS-S5 (21Aug)
Tracking Status
firefox43 --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 3 obsolete files)

We have had an history of device getting slows and slower because of mozSettings observers being leaked. We should be more defensive. Currently, investigating of this do not scales:
 - notice your device is slow
 - get about:memory report
 - search for "settings-observers-suspect"

This is based on having at least 20 callbacks registered on the same key for the same living app code.

We can improve this:
 - lower the threshold from 20 to 10
 - issue a warning in the console to advise developpers
 - maybe hit an assertion or throw when this happens on debug build
 - dump the stack when this happens
Attached patch Make mozSettings more defensive (obsolete) — Splinter Review
Attachment #8646533 - Flags: review?(anygregor)
Comment on attachment 8646533 [details] [diff] [review]
Make mozSettings more defensive

This is implementing everything suggested, including always throwing. Now that might be risky and currently not in sync with the WebIDL.

Maybe we should only throw in debug builds ?
Flags: needinfo?(anygregor)
Attachment #8646533 - Flags: review?(anygregor)
Comment on attachment 8646533 [details] [diff] [review]
Make mozSettings more defensive

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

::: dom/settings/SettingsManager.js
@@ +381,5 @@
> +      debug("WARNING: MORE THAN " + kMaxObservers + " OBSERVERS FOR " +
> +            aName + ": " + (new Error).stack);
> +      throw Components.results.NS_ERROR_ABORT;
> +    }
> +

Lets just put the real number (length) in the error message if we have it.
And lets not throw but just put it in logcat.

::: dom/settings/tests/test_settings_observer_killer.html
@@ +24,5 @@
> +SpecialPowers.addPermission("settings-api-read", true, document);
> +SpecialPowers.addPermission("settings-api-write", true, document);
> +SpecialPowers.addPermission("settings-read", true, document);
> +SpecialPowers.addPermission("settings-write", true, document);
> +SpecialPowers.addPermission("settings-clear", true, document);

Shouldn't we do something like http://mxr.mozilla.org/mozilla-central/source/dom/contacts/tests/shared.js#499 here as well to avoid races?
Attachment #8646533 - Flags: feedback+
Flags: needinfo?(anygregor)
Attachment #8647004 - Flags: review?(anygregor)
Comment on attachment 8647004 [details] [diff] [review]
Make mozSettings more defensive

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

::: dom/settings/SettingsManager.js
@@ +38,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "uuidgen",
>                                     "@mozilla.org/uuid-generator;1",
>                                     "nsIUUIDGenerator");
>  
> +const kMaxObservers = 10;

Lets change the name here since its not a hard limit. maybe something like kObserverSoftLimit

::: dom/settings/tests/test_settings_observer_killer.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=XXX

Fixxxme here and below!

@@ +19,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript;version=1.7">
> +
> +var url = SimpleTest.getTestFileURL("file_loadserver.js");
> +var script = SpecialPowers.loadChromeScript(url);

Is this needed?
Attachment #8647004 - Flags: review?(anygregor) → review+
(In reply to Gregor Wagner [:gwagner] from comment #7)
> Comment on attachment 8647004 [details] [diff] [review]
> Make mozSettings more defensive
> 
> Review of attachment 8647004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/settings/SettingsManager.js
> @@ +38,5 @@
> >  XPCOMUtils.defineLazyServiceGetter(this, "uuidgen",
> >                                     "@mozilla.org/uuid-generator;1",
> >                                     "nsIUUIDGenerator");
> >  
> > +const kMaxObservers = 10;
> 
> Lets change the name here since its not a hard limit. maybe something like
> kObserverSoftLimit

Sure.

> 
> ::: dom/settings/tests/test_settings_observer_killer.html
> @@ +1,4 @@
> > +<!DOCTYPE html>
> > +<html>
> > +<!--
> > +https://bugzilla.mozilla.org/show_bug.cgi?id=XXX
> 
> Fixxxme here and below!

:)

> 
> @@ +19,5 @@
> > +<pre id="test">
> > +<script class="testbody" type="text/javascript;version=1.7">
> > +
> > +var url = SimpleTest.getTestFileURL("file_loadserver.js");
> > +var script = SpecialPowers.loadChromeScript(url);
> 
> Is this needed?

Yes we need it so that SettingsRequestManager exists.
Whiteboard: [systemsfe]
Target Milestone: --- → FxOS-S5 (21Aug)
https://hg.mozilla.org/mozilla-central/rev/39c16acfb348
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 1203999
You need to log in before you can comment on or make changes to this bug.