Closed Bug 1447544 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Toolkit
Component:
Video/Audio Controls
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(2 files)

Bug 1447544 - Remove the event listeners and timers when the videocontrols binding is destroyed
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1447544 - Set firstShow to false before the control fades out
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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.
triage: adding to backlog. Are you planning to work on this Tim?
Flags: needinfo?(timdream)
Priority: -- → P3
Sure, but after a few XBL bugs.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Flags: needinfo?(timdream)
Attachment #8963038 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8963038 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8965224 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963038 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963038 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8965224 [details]
Bug 1447544 - Set firstShow to false before the control fades out

https://reviewboard.mozilla.org/r/233940/#review239660
Attachment #8965224 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8963038 - Flags: review?(gijskruitbosch+bugs) → review+
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
Pushed by timdream@gmail.com:
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
Pushed by timdream@gmail.com:
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
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.
Flags: needinfo?(timdream)
https://hg.mozilla.org/mozilla-central/rev/a172658a9b2b
https://hg.mozilla.org/mozilla-central/rev/decb7e619d1a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: