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)

ARM
Gonk (Firefox OS)
defect

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)
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?
> I/Gecko ( 9975): -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR > dev.gaia.pinning_the_web: 17 Doh, nm :)
Is this fixed by bug 1193051?
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 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.
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+
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)
Attachment #8660405 - Flags: feedback?(21) → review?(21)
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)
Priority: -- → P2
Attachment #8660405 - Flags: review?(21)
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.
Blocks: 1197865
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 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+
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.

Attachment

General

Created:
Updated:
Size: