961793 - BrowserElementChildPreload/BrowserElementPanning need to unregister observers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: 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?

Comment 2

[[:fabrice] Fabrice Desré]

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

Comment 3

[Nathan Froyd [:froydnj]]

(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.

Comment 4

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70967d2e42ba

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70967d2e42ba

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: unregister observers in BrowserElementPanning.js at global unload

...and here's the equivalent patch for BrowserElementPanning.js.

Comment 7

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b10b085ab73c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b10b085ab73c

Comment 9

[Jason Smith [:jsmith]]

Blocks a blocker. Let's uplift this to 1.3.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/662ce8f620a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/299ddf0c23a8