Closed
Bug 1203999
Opened 9 years ago
Closed 9 years ago
Pin the Web is leaking setting observers
Categories
(Firefox OS Graveyard :: Gaia::System, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: vnicolas, Assigned: apastor)
References
Details
(Whiteboard: [systemsfe])
Attachments
(1 file)
Happens to me just opening/closing app while hacking.
I/Gecko ( 9975): -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR dev.gaia.pinning_the_web: 17 FROMaddObserver@jar:file:///system/b2g/omni.ja!/components/SettingsManager.js:382:48
I/Gecko ( 9975): sl_observe@app://system.gaiamobile.org/shared/js/settings_listener.js:58:5
I/Gecko ( 9975): BrowserContextMenu@app://system.gaiamobile.org/js/browser_context_menu.js:21:1
I/Gecko ( 9975): BaseModule.instantiate/constructor@app://system.gaiamobile.org/js/base_module.js:471:9
I/Gecko ( 9975): BaseModule.instantiate@app://system.gaiamobile.org/js/base_module.js:474:1
I/Gecko ( 9975): aw_installSubComponents@app://system.gaiamobile.org/js/app_window.js:863:1
I/Gecko ( 9975): aw_render@app://system.gaiamobile.org/js/app_window.js:702:5
I/Gecko ( 9975): AppWindow@app://system.gaiamobile.org/js/app_window.js:65:5
I/Gecko ( 9975): AppWindowFactory.prototype.trackLauchingWindow@app://system.gaiamobile.org/js/app_window_factory.js:230:17
I/Gecko ( 9975): awf_launch/launchApp@app://system.gaiamobile.org/js/app_window_factory.js:216:11
I/Gecko ( 9975): awf_launch@app://system.gaiamobile.org/js/app_window_factory.js:224:11
I/Gecko ( 9975): awf_handleEvent@app://system.gaiamobile.org/js/app_window_factory.js:125:11
Flags: needinfo?(apastor)
Comment 1•9 years ago
|
||
What in this stack trace makes you finger Pin the Web as the source of this leak? Is this coming from related changes in AppChrome?
Comment 2•9 years ago
|
||
> I/Gecko ( 9975): -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR
> dev.gaia.pinning_the_web: 17
Doh, nm :)
Comment 3•9 years ago
|
||
Is this fixed by bug 1193051?
Assignee | ||
Comment 4•9 years ago
|
||
Nope. I'll take this one.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> Is this fixed by bug 1193051?
Assignee: nobody → apastor
Flags: needinfo?(apastor)
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
Comment on attachment 8660405 [details]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master
Applied locally, it seems to do the job as expected :)
Attachment #8660405 -
Flags: feedback+
You may want to double check that once the BrowserContextMenu.show method has been called and the app is killed by a memory pressure the BrowserContextMenu.hide method is really called, which is unclear looking at the code (but maybe there is an abstraction that does that).
Otherwise we may still leak in some cases.
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8660405 [details]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master
Vivien, could you please review this? Thanks
Attachment #8660405 -
Flags: review?(21)
Comment on attachment 8660405 [details]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master
f+ but I'm not sure the call to hideBrowserContextMenu covers all the cases here.
For example does it covers app killed by the user from the card view ? If yes consider my f+ to be a r+ :)
Attachment #8660405 -
Flags: review?(21) → feedback+
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8660405 [details]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master
You are totally right. I assumed the context menu was being hidden when entering in the cards view.
Attachment #8660405 -
Flags: feedback+ → feedback?(21)
Assignee | ||
Updated•9 years ago
|
Attachment #8660405 -
Flags: feedback?(21) → review?(21)
Reporter | ||
Comment 11•9 years ago
|
||
Comment on attachment 8660405 [details]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master
Sorry for all this back and forth. I realized that this solution may be a bit racy if the user quickly open the contextmenu and click on something and also I would prefer and explicit call to this.contextmenu.destroy() in the destroy() method of AppWindow.
It makes it clearer that the ContextMenu may have some stuff to clear, instead of a call to this.contextmenu.hide() which is a bit implicit and may let someone change the code in the future to avoid calling this code if the contextmenu is not visible.
So I think you are close to finish the patch and this should be the last iteration!
Attachment #8660405 -
Flags: review?(21)
Updated•9 years ago
|
Priority: -- → P2
Assignee | ||
Updated•9 years ago
|
Attachment #8660405 -
Flags: review?(21)
Assignee | ||
Comment 12•9 years ago
|
||
After thinking again about it, I believe is safer just getting the preference (and unobserve) every time the context menu items list is generated. Creating the observer in the constructor would result on the initial bug, as the context menu is instantiated (without showing) many times.
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8660405 [details]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master
Sorry for the rush, but is blocking some bugs... Etienne, could you please take a look? Thanks!
Attachment #8660405 -
Flags: review?(etienne)
Comment 14•9 years ago
|
||
Comment on attachment 8660405 [details]
[gaia] albertopq:1203999-settings-leaking > mozilla-b2g:master
r=me with the last commit addressed :) Thanks!
Attachment #8660405 -
Flags: review?(etienne)
Attachment #8660405 -
Flags: review?(21)
Attachment #8660405 -
Flags: review+
Assignee | ||
Comment 15•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•