Closed Bug 1274879 Opened 3 years ago Closed 3 years ago

Event handlers are not removed properly for video controls after reflow

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

Details

Attachments

(2 files)

Attached file testcase
Steps to reproduce:
1. open the testcase
2. focus on the video
3. press down arrow
4. wait for a second then press up arrow

Expected result:
the volumn should only change 10% for step 3 and step 4

Actual result:
the volumn changes 100% for step 3 and step 4


In this testcase page, there is a background timer tries to trigger reflow on the video element 10 times every second, which causes 10 new keypress handlers be added on the video from the videocontrols widget. So any key press would be duplicate at least 10 times.

In normal cases, accumulated handlers would be removed after this volumn change because terminateEventListeners would be called by handleEvent. But reflow accumulates handlers is still an issue, because reflow can happen at any time without user's interaction with the video, and large number of event handlers can slow down the page.

Also note that, fullscreenchange handler is affected as well. Actually I found this issue from fullscreenchange.
Jared, is this something you can poke at?
Flags: needinfo?(jaws)
It's a bug to fix, but seems like something unlikely a user is going to run across in the wild?
Flags: needinfo?(jaws)
(In reply to Justin Dolske [:Dolske] from comment #2)
> It's a bug to fix, but seems like something unlikely a user is going to run
> across in the wild?

Well, the testcase is 100% reproducible, and unlikely. But see also bug 1187296 - we seem to have issues with how the binding behaves, which is a bit worrying (and certainly bug 1187296 is something that users do run into).
The testcase is made to be highly reproducible and significant.

I'd suggest user can easily hit this issue, though not so significantly. Maybe at most they could observe a 20% or 30% change for one key stroke rather than 100% here. But that uncertainty is absolutely a bug should be fixed.

Volume change may not be that obvious, but time change could be more annoying I suppose. They may see a 30s or 45s backward / forward rather than the expected 15s. (Actually I'd prefer 5s rather than 15s, but that's a different topic.)

Given it is hard to stably reproduce, I suspect user would tend not to report this kind of issues. So you never know how many users ever get annoyed by this.
Also this issue matters a lot if you are testing performance of fullscreen change, because fullscreen change triggers reflow but does not trigger terminateEventListeners, which means every time you enter fullscreen and exit, you would accumulate two fullscreenchange handler, and after several cycles, the fullscreenchange handler would be called tens of times for every single click.
Does the constructor fire each time we rebind the binding? Feels like we could just remove the event listeners there before adding them? Worst case scenario, that's just going to no-op.
Flags: needinfo?(bugzilla)
(In reply to :Gijs Kruitbosch from comment #6)
> Does the constructor fire each time we rebind the binding? Feels like we
> could just remove the event listeners there before adding them? Worst case
> scenario, that's just going to no-op.

Sounds like yes, but how can we call methods of the detached binding instance?
Flags: needinfo?(bugzilla)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > Does the constructor fire each time we rebind the binding? Feels like we
> > could just remove the event listeners there before adding them? Worst case
> > scenario, that's just going to no-op.
> 
> Sounds like yes, but how can we call methods of the detached binding
> instance?

I don't think we need to. There's a Utils object that's assigned onto the element as an expando, so presumably we can access that (and call terminateEventListeners or whatever on it) before we assign the new one (I *think* we do need a new one because all the anonymous content element references will be dead).
Then could you take this and try that way? I'm not familiar with this part of code and would like to learn from your patch.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> Then could you take this and try that way?

I don't really have the time to do that.

> I'm not familiar with this part
> of code and would like to learn from your patch.

I'm happy to mentor this. I *think* it's basically just a question of inserting something like:

if (this.Utils) {
  this.Utils.terminateEventListeners();
}

before assigning to this.Utils, in the constructor of the binding. Does that make sense? Is there a reason that doesn't work?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugzilla)
It shouldn't work... "this" points to different object for each binding, doesn't it? Otherwise the condition "this.videocontrols.randomID != this.randomID" in handleEvent would never work. Probably we can set something to this.videocontrols and detect that, but I'm not sure whether attaching an object to that would work either.
Flags: needinfo?(bugzilla)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> It shouldn't work... "this" points to different object for each binding,
> doesn't it? Otherwise the condition "this.videocontrols.randomID !=
> this.randomID" in handleEvent would never work. Probably we can set
> something to this.videocontrols and detect that, but I'm not sure whether
> attaching an object to that would work either.

I don't think so... In the constructor (and any XBL methods), |this| is the videocontrols element in question. The handleEvent method is not on the element, but on the Utils object, and so |this| is the Utils object, and this.videocontrols refers back to the element.

When the binding is rebound, we create a new Utils object and we update the expando on the element and the randomID of the new Utils object to be the same.

Now, when the old Utils object's handleEvent method gets called because of the old event listener, Utils.videocontrols.randomID (this.videocontrols.randomID) has been updated, but Utils.randomID (this.randomID) has not (because it's the old Utils object that's been kept alive by the event listener, and it hasn't been changed when the new Utils object was constructed).

Of course, it's possible I'm still misunderstanding how the code works - again, I've not tested this. Can you sanity-check my logic? :-)
Flags: needinfo?(bugzilla)
You explanation makes sense... but the solution doesn't work. I actually had tried before posting that comment.

So I add the following lines:
> if (this.Utils) {
>     this.Utils.terminateEventListeners();
> }
before "this.Utils = {", but the issue is still reproducible with the steps in comment 0.

Adding "console.log(typeof this.Utils);" before that, the console would continously shows "undefined" which indicates that value is not set.
Flags: needinfo?(bugzilla)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> You explanation makes sense... but the solution doesn't work. I actually had
> tried before posting that comment.
> 
> So I add the following lines:
> > if (this.Utils) {
> >     this.Utils.terminateEventListeners();
> > }
> before "this.Utils = {", but the issue is still reproducible with the steps
> in comment 0.
> 
> Adding "console.log(typeof this.Utils);" before that, the console would
> continously shows "undefined" which indicates that value is not set.

That's... interesting. It seems like this.Utils should be an expando on the element, and I don't know why it would get unset (or, if it does, why the randomID is not also getting unset... though I guess the inequality might still work if it got unset, too?). At that stage, is this.randomID also undefined ?
Flags: needinfo?(bugzilla)
At the very beginning of the constructor, yes, both are undefined.

I'm confused as well, then...
Flags: needinfo?(bugzilla)
Oh, I guess randomID is removed in the destructor, and that's why the inequality works.
OK, adding
> this.Utils.terminateEventListeners();
to the destructor works. Not sure why it wasn't done that way.

I'll work on a full patch tomorrow.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> OK, adding
> > this.Utils.terminateEventListeners();
> to the destructor works. Not sure why it wasn't done that way.

The destructor doesn't always run, IME. We might need to keep the existing approach in addition to adding this to the destructor.
It seems to me if the destructor doesn't run, the randomID would not be deleted, and the existing approach wouldn't work either.

Do you know in which case the destructor would not run?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #19)
> It seems to me if the destructor doesn't run, the randomID would not be
> deleted, and the existing approach wouldn't work either.

Why wouldn't it? Seems like the old ID that was on the element as an expando, and on the old copy of Utils as a property, would still be different from the new one that we generate when the constructor runs again, so once the old copy of Utils gets called, the expando on the element that it checks against will be different, no?

> Do you know in which case the destructor would not run?

bug 230086
bug 740949
bug 1151975

Not sure if/which of these would happen here, but I'd be nervous about relying only on the destructor.

It's also confusing that based on what you've said, we're saying that the element's randomID expando gets deleted because of the destructor, but the Utils expando gets deleted because... ?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugzilla)
In fact, does the video control element maybe get re-created in this testcase? That would explain things. Because the event listeners are on the video element, but the expando tracking is on the videocontrols element, as is the binding. That would also mean we're leaking the old videocontrols element(s) because of the event listeners that stay attached to the <video> itself.
Oh, wait... yes. The testcase is not a basic reflow, but a reflow with frame reconstruction, in which case, a new videocontrols element would definitely be created. A reflow with frame reconstruction doesn't happen as frequently as reflow without that, so this is less concerned as well. Fullscreen change always trigger frame reconstruction, though.

So yes, that means we're leaking the old videocontrols element.
Flags: needinfo?(bugzilla)
It seems to me the easiest way to add test for this would be adding another one in test_videocontrols.html, which I'm touching in bug 1274520. I'm going to wait until that bug gets landed.
Comment on attachment 8759120 [details]
MozReview Request: Bug 1274879 - Terminate event listeners of videocontrols in destructor. r?jaws

https://reviewboard.mozilla.org/r/57180/#review54854
Attachment #8759120 - Flags: review?(jaws) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e3d8982f1a5
Terminate event listeners of videocontrols in destructor. r=jaws
https://hg.mozilla.org/mozilla-central/rev/5e3d8982f1a5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.