748077 - Remove some leaks in b2g/

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

There is still one leak in dom/system/gonk/ril_worker.js but this is a different bug.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) from comment #1)
> There is still one leak in dom/system/gonk/ril_worker.js but this is a
> different bug.

Sorry, I meant dom/system/gonk/RadioInterfaceLayer.js

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 617606 [details] [diff] [review]
Patch

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

::: b2g/chrome/content/shell.js
@@ +137,5 @@
>      browser.goHome();
>    },
>  
>    stop: function shell_stop() {
> +    ['keydown', 'keypress', 'keyup'].forEach((function unlistenKey(type) {

Nit: if we expect it to grow, move the event list to a const and use it both to add and remove event listeners.

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 617606 [details] [diff] [review]
Patch

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

::: b2g/chrome/content/shell.js
@@ +15,5 @@
>  Cu.import('resource://gre/modules/Webapps.jsm');
>  
> +XPCOMUtils.defineLazyServiceGetter(Services, 'env',
> +                                   '@mozilla.org/process/environment;1',
> +                                   'nsIEnvironment');

If we're touching these lines, I would really prefer that we stop monkey patching Services.jsm. It's just bad form.

Can't we just add those to Services.jsm proper, or alternatively define these as local globals or in a B2GServices object?

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to Vivien Nicolas (:vingtetun) from comment #2)
> (In reply to Vivien Nicolas (:vingtetun) from comment #1)
> > There is still one leak in dom/system/gonk/ril_worker.js but this is a
> > different bug.
> 
> Sorry, I meant dom/system/gonk/RadioInterfaceLayer.js

Interesting. Can you please file? Thanks!

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Philipp von Weitershausen [:philikon] from comment #5)
> (In reply to Vivien Nicolas (:vingtetun) from comment #2)
> > (In reply to Vivien Nicolas (:vingtetun) from comment #1)
> > > There is still one leak in dom/system/gonk/ril_worker.js but this is a
> > > different bug.
> > 
> > Sorry, I meant dom/system/gonk/RadioInterfaceLayer.js
> 
> Interesting. Can you please file? Thanks!

Yup, I have opened bug 751957

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a908256db5f

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4a908256db5f