Closed
Bug 1193469
Opened 9 years ago
Closed 9 years ago
Make mozSettings more defensive on addObserver
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
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)
|
(deleted),
patch
|
gerard-majax
:
review+
|
Details | Diff | Splinter Review |
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
|
Assignee | |
Comment 1•9 years ago
|
||
This is a first try with throwing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df5b5fcdfa3d
|
|
Assignee | |
Comment 2•9 years ago
|
||
Attachment #8646533 -
Flags: review?(anygregor)
|
|
Assignee | |
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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+
|
|
||
Updated•9 years ago
|
Flags: needinfo?(anygregor)
|
|
Assignee | |
Comment 5•9 years ago
|
||
Attachment #8646533 -
Attachment is obsolete: true
Attachment #8646956 -
Flags: review?(anygregor)
|
|
Assignee | |
Comment 6•9 years ago
|
||
Attachment #8646956 -
Attachment is obsolete: true
Attachment #8646956 -
Flags: review?(anygregor)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8647004 -
Flags: review?(anygregor)
|
|
||
Comment 7•9 years ago
|
||
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+
|
|
Assignee | |
Comment 8•9 years ago
|
||
(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.
|
|
Assignee | |
Comment 9•9 years ago
|
||
Attachment #8647004 -
Attachment is obsolete: true
Attachment #8647744 -
Flags: review+
|
|
Assignee | |
Comment 10•9 years ago
|
||
Keywords: checkin-needed
|
||
Comment 11•9 years ago
|
||
Keywords: checkin-needed
|
|
||
Updated•9 years ago
|
Whiteboard: [systemsfe]
Target Milestone: --- → FxOS-S5 (21Aug)
|
||
Comment 12•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•