1447544 - Audit the event listeners and timers; make sure they are properly cleaned up

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

While working on bug 1444489 it is found that not all event listeners get cleaned up when the binding is destroyed. The timers should also too.

Beware of the effect caused by bug 718107 though.

Comment 1

[Sam Foster [:sfoster] (he/him)]

triage: adding to backlog. Are you planning to work on this Tim?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sure, but after a few XBL bugs.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

Additionally, remove `self = this` usage, and access to the `Utils` object from the scope chain.

Review commit: https://reviewboard.mozilla.org/r/231874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231874/

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231874/diff/1-2/

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231874/diff/2-3/

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231874/diff/3-4/

Comment 7

[:Gijs (out for now; he/him)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

https://reviewboard.mozilla.org/r/231874/#review239430

This is already nice, but I'd like to see it again after the changes/questions below have been addressed.

::: toolkit/content/widgets/videocontrols.xml:629
(Diff revision 4)
> +              case this.playButton:
> +                this.clickToPlayClickHandler(aEvent);
> +                break;
> +              case this.clickToPlay:
> +                this.clickToPlayClickHandler(aEvent);
> +                break;
> +              case this.textTrackList:
> +                const index = +aEvent.originalTarget.getAttribute("index");
> +                this.changeTextTrack(index);
> +                break;
> +              case this.controlsSpacer:
> +                this.clickToPlayClickHandler(aEvent);
> +                break;

The controlsSpacer, clickToPlay and playButton ones could use a fall-through to reduce duplication here.

::: toolkit/content/widgets/videocontrols.xml:655
(Diff revision 4)
> +          case "fullscreenchange":
> +            this.onFullscreenChange();

Nit: please add a comment here too.

::: toolkit/content/widgets/videocontrols.xml:717
(Diff revision 4)
> -              element.item.removeEventListener(element.event, element.func,
> -                { mozSystemGroup: element.mozSystemGroup, capture: element.capture });
> +          this.muteButton.removeEventListener("click", this, { mozSystemGroup: true });
> +          this.castingButton.removeEventListener("click", this, { mozSystemGroup: true });
> +          this.closedCaptionButton.removeEventListener("click", this, { mozSystemGroup: true });
> +          this.fullscreenButton.removeEventListener("click", this, { mozSystemGroup: true });
> +          this.playButton.removeEventListener("click", this, { mozSystemGroup: true });
> +          this.clickToPlay.removeEventListener("click", this, { mozSystemGroup: true });
> +          this.textTrackList.removeEventListener("click", this, { mozSystemGroup: true });
> +
> +          this.controlsSpacer.removeEventListener("click", this, { mozSystemGroup: true });

Seems like we could reduce a lot of duplication here (and where we add the listeners) by having a structure that listed the [item (key), eventName] pairs that we added/removed listeners for. We use system event groups and `this` as the listener throughout, so this would become shorter and easier to read, plus we'd know for sure that every listener we add also gets removed.

::: toolkit/content/widgets/videocontrols.xml:749
(Diff revision 4)
> -            } catch (ex) {}
> +        } catch (ex) {}
> -          }
>  
> -          delete this.controlListeners;
> -        }
> +        clearTimeout(this._showControlsTimeout);
> +        clearTimeout(this._hideControlsTimeout);
> +        this._cancelShowThrobberWhileResumingVideoDecoder();

This seems to be new (at least, find in page is not finding me other previous callsites where this moved from). Can you elaborate on why it's necessary?

::: toolkit/content/widgets/videocontrols.xml:2077
(Diff revision 4)
> +            break;
> +          case "mouseup":
> +            if (aEvent.originalTarget == this.Utils.controlsSpacer) {
> +              if (this.firstShow) {
> +                this.Utils.video.play();
> -          this.firstShow = false;
> +                this.firstShow = false;

Can you elaborate on why the transitionend stuff (which I think became dead in bug 1449532 right?) is now inside a mouseup handler? It's not obvious from the context and it feels like something that should maybe be its own change, with its own commit message.

::: toolkit/content/widgets/videocontrols.xml:2210
(Diff revision 4)
>          }
>  
> -        for (let element of this.controlListeners) {
> -          try {
> +        try {
> -            element.item.removeEventListener(element.event, element.func,
> -              { mozSystemGroup: true });
> +          this.clickToPlay.removeEventListener("click", this, { mozSystemGroup: true });
> +          this.video.removeEventListener("MozNoControlsBlockedVideo", this, { mozSystemGroup: true });

Shouldn't this be a video event?

::: toolkit/content/widgets/videocontrols.xml:2290
(Diff revision 4)
> -        addListener(this.clickToPlay, "click", this.clickToPlayClickHandler);
> -        addListener(this.video, "MozNoControlsBlockedVideo", this.blockedVideoHandler);
>  
>          for (let event of this.videoEvents) {
> -          this.video.addEventListener(event, this, { mozSystemGroup: true });
> +          this.video.addEventListener(event, this, {
> +            capture: true,

Why the switch to capturing listeners?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

https://reviewboard.mozilla.org/r/231874/#review239430

> This seems to be new (at least, find in page is not finding me other previous callsites where this moved from). Can you elaborate on why it's necessary?

Because we want to clear the timers in this patch? This method cancels `this._showThrobberTimer` timeout.

> Can you elaborate on why the transitionend stuff (which I think became dead in bug 1449532 right?) is now inside a mouseup handler? It's not obvious from the context and it feels like something that should maybe be its own change, with its own commit message.

Yeah, this should go to another changeset. Sorry about not documenting that on this changeset.

> Why the switch to capturing listeners?

For no reason other than consistency.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1447544 - Set firstShow to false before the control fades out

The flag is used to trigger play() when the control first hides.
There is no reason to set only after transition ends.  We can set it to false
as soon as we trigger play().

This is likely to fix a regression from bug 1449532 given that we have switched
to Web Animation API there.

Review commit: https://reviewboard.mozilla.org/r/233940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233940/

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231874/diff/4-5/

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8965224 [details]
Bug 1447544 - Set firstShow to false before the control fades out

https://reviewboard.mozilla.org/r/233940/#review239660

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

https://reviewboard.mozilla.org/r/231874/#review239666

r=me with 2 minor nits below addressed.

::: toolkit/content/widgets/videocontrols.xml:2076
(Diff revision 5)
> -            this.Utils.video.removeEventListener(event, this);
> -          } catch (ex) {}
> +          for (let { el, type, mozSystemGroup = true, capture = false } of this.controlsEvents) {
> +            el.removeEventListener(type, this, { mozSystemGroup, capture });

None of these use the `capture` property so we could just omit it.

::: toolkit/content/widgets/videocontrols.xml:2096
(Diff revision 5)
> -        this.Utils.controlsSpacer.addEventListener("mouseup", function(event) {
> -          if (event.originalTarget == self.Utils.controlsSpacer) {
> +        for (let { el, type, mozSystemGroup = true, capture = false } of this.controlsEvents) {
> +          el.addEventListener(type, this, { mozSystemGroup, capture });

Ditto.

::: toolkit/content/widgets/videocontrols.xml:2275
(Diff revision 5)
>          for (let event of this.videoEvents) {
> -          this.video.addEventListener(event, this, { mozSystemGroup: true });
> +          this.video.addEventListener(event, this, {
> +            capture: true,
> +            mozSystemGroup: true
> +          });
>          }

So my understanding is that you're adding these video event listeners capturing now, when they weren't before. I *think* this is technically a web-related change, in that we will now change the visibiltiy of the noControlsOverlay earlier than we used to, in case web content is also listening to the same events in the non-system group, bubbling phase. I don't think this matters in practice, but it's probably good to note explicitly this is happening.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

https://reviewboard.mozilla.org/r/231874/#review239666

> So my understanding is that you're adding these video event listeners capturing now, when they weren't before. I *think* this is technically a web-related change, in that we will now change the visibiltiy of the noControlsOverlay earlier than we used to, in case web content is also listening to the same events in the non-system group, bubbling phase. I don't think this matters in practice, but it's probably good to note explicitly this is happening.

If I understand `EventTargetChainItem::HandleEventTargetChain()` correctly, events are dispatched in the following phases:

1. non-system group capture
2. non-system group target
3. non-system group bubble
4. system group capture
5. system group target
6. system group bubble

Web content should not be aware of our move from 6 to 4.

https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/dom/events/EventDispatcher.cpp#590

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231874/diff/5-6/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31af64e9e325
Set firstShow to false before the control fades out r=Gijs
https://hg.mozilla.org/integration/autoland/rev/177731fbf802
Remove the event listeners and timers when the videocontrols binding is destroyed r=Gijs

Comment 16

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (bug 1447544) for dt failures in devtools/client/debugger/test/mochitest/browser_dbg_event-listeners-03.j on a CLOSED TREE

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=177731fbf8029dc6e5800db1e6451724ac90df26&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&filter-searchStr=dt6&selectedJob=172104450

Log failure: https://treeherder.mozilla.org/logviewer.html#?job_id=172104450&repo=autoland&lineNumber=2703

Backout: https://hg.mozilla.org/integration/autoland/rev/1755df4f48bb18ec3a552a51696656e33b8e6c52

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8963038 [details]
Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231874/diff/6-7/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a172658a9b2b
Set firstShow to false before the control fades out r=Gijs
https://hg.mozilla.org/integration/autoland/rev/decb7e619d1a
Remove the event listeners and timers when the videocontrols binding is destroyed r=Gijs

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry for not spotting that in Try. I did a |./mach try -b do -p all -u all -t none --artifact| and the actual error was hidden in many other errors caused by trying to run binary tests on an artifact build.

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a172658a9b2b
https://hg.mozilla.org/mozilla-central/rev/decb7e619d1a