BrowserElementChildPreload/BrowserElementPanning need to unregister observers

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 1 bug, {memory-leak, perf})

unspecified
mozilla29
memory-leak, perf
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [caf priority: p3][c=memory p= s=2014.01.31 u=1.3][qa-][cr 606932])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
The observers added here:

http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#256
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#61

never get removed and build up over the course of mochitest runs.

Please note that the correct solution here is not to make the BrowserElementPanning observers weak observers; we should instead make the BrowserElementChildPreload observers strong observers and then properly manage the lifetime of all the observers.
(Assignee)

Comment 1

4 years ago
Created attachment 8362677 [details] [diff] [review]
don't register weak observers in BrowserElementChildPreload.js

Turns out we can just remove the observers at unload.

I'm less sure of how to address BrowserElementPanning's observers, as
BrowserElementChildPreload can't (I don't think) see the ContentPanning
variable defined therein.  Registering an unload handler for ContentPanning
would work, I suppose.  Do you have any preferred course of action here?
Attachment #8362677 - Flags: review?(fabrice)
Comment on attachment 8362677 [details] [diff] [review]
don't register weak observers in BrowserElementChildPreload.js

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

I'm fine with trying to use an unload handler for BEPanning too.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +100,5 @@
>  
>    return null;
>  }
>  
> +const OBSERVED_EVENTS = [

I would prefer kObservedEvents, but no big deal if you want to stick with UPPER_CASE
Attachment #8362677 - Flags: review?(fabrice) → review+
(Assignee)

Comment 3

4 years ago
(In reply to Fabrice Desré [:fabrice] from comment #2)
> I'm fine with trying to use an unload handler for BEPanning too.

OK, great, I'll just add that in a separate patch.

> ::: dom/browser-element/BrowserElementChildPreload.js
> > +const OBSERVED_EVENTS = [
> 
> I would prefer kObservedEvents, but no big deal if you want to stick with
> UPPER_CASE

I went with UPPER_CASE because that seemed to be the prevailing style in that file.
(Assignee)

Comment 4

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/70967d2e42ba
Flags: in-testsuite-
Whiteboard: [leave open]
(Assignee)

Comment 6

4 years ago
Created attachment 8363968 [details] [diff] [review]
unregister observers in BrowserElementPanning.js at global unload

...and here's the equivalent patch for BrowserElementPanning.js.
Attachment #8363968 - Flags: review?(fabrice)
Attachment #8363968 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/b10b085ab73c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Blocks: 964386
Blocks a blocker. Let's uplift this to 1.3.
blocking-b2g: --- → 1.3+
https://hg.mozilla.org/releases/mozilla-aurora/rev/662ce8f620a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/299ddf0c23a8
status-b2g-v1.3: --- → fixed
status-b2g-v1.4: --- → fixed
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
status-firefox29: --- → fixed
No longer blocks: 964386
Duplicate of this bug: 964386

Updated

4 years ago
Keywords: mlk, perf
Priority: -- → P1
Whiteboard: [c=memory p= s= u=1.3]

Updated

4 years ago
Whiteboard: [c=memory p= s= u=1.3] → [c=memory p= s=2014.01.31 u=1.3]

Updated

4 years ago
Whiteboard: [c=memory p= s=2014.01.31 u=1.3] → [c=memory p= s=2014.01.31 u=1.3][qa-]
Whiteboard: [c=memory p= s=2014.01.31 u=1.3][qa-] → [c=memory p= s=2014.01.31 u=1.3][qa-][cr 606932]

Updated

4 years ago
Whiteboard: [c=memory p= s=2014.01.31 u=1.3][qa-][cr 606932] → [caf priority: p3][c=memory p= s=2014.01.31 u=1.3][qa-][cr 606932]
Assignee: nobody → nfroyd
status-b2g-v1.3T: --- → fixed
You need to log in before you can comment on or make changes to this bug.