1613069 - When closing the browser, see JavaScript error: resource://gre/actors/PictureInPictureChild.jsm, line 414: TypeError: can't access property "disarm", state.mousemoveDeferredTask is null

Status: VERIFIED FIXED

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

Description

[:Gijs (he/him)]

This is coming from https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/toolkit/actors/PictureInPictureChild.jsm#414 which is probably being called by willDestroy. The mousemoveDeferredTask is initialized as null, so this code should probably check that before trying to use it.

I suspect this was hidden by bug 1596187 until that got fixed.

I suspect from the other stuff this code is doing that this could theoretically leak things when closing windows without closing the browser.

Comment 1

[:Gijs (he/him)]

More precise STR:

1. open browser
2. open https://www.mozilla.org/media/img/home/2018/billboard-more-power-high-res.5e9e07e1024c.png
3. right click on the image
4. dismiss context menu
5. quit browser

I don't know why these steps trip this. I suspect something triggers the creation of the child actor and otherwise the actor doesn't even start so we don't hit this path.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1613069 - don't trip over null mousemove tasks when destroying PictureInPictureChild actors, r?jaws

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #0)

I suspect from the other stuff this code is doing that this could theoretically leak things when closing windows without closing the browser.

Specifically, although the rest of the stopTrackingMouseOverVideos doesn't really need to run here because beginTrackingMouseOverVideos hasn't run and nothing will leak from there, inside willDestroy we're removing a pref observer that looks like it might keep the JS wrapper etc. for the actor alive...

I wonder if we can just define a lazy pref getter as a global instead of having one observer per child actor. I filed bug 1613082 for this.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef9ca2da9e69
don't trip over null mousemove tasks when destroying PictureInPictureChild actors, r=jaws

Comment 5

[Rob Wu [:robwu]]

Gijs, shouldn't the condition be if (!state.isTrackingVideos) { rather than if (!state.mousemoveDeferredTask) {?

The isTrackingVideos flag is always toggled at the start/end of those methods, whereas mousemoveDeferredTask is initialized once and never cleared after disarming.

Comment 7

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #5)

Gijs, shouldn't the condition be if (!state.isTrackingVideos) { rather than if (!state.mousemoveDeferredTask) {?

The isTrackingVideos flag is always toggled at the start/end of those methods, whereas mousemoveDeferredTask is initialized once and never cleared after disarming.

Hm, I missed that bool. It's probably slightly cleaner to do it that way, though I don't think it makes a practical difference - the event listener removals etc. will just be no-ops, and I have to admit I was nervous about accidentally not running things when we should be running them... in any case, I filed bug 1613786 and hopefully when Mike is back he can give a conclusive answer - perhaps there is a saner solution here, this all feels a bit fragile.

Comment 8

[Cosmin Sabou [:CosminS]]

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

Comment 9

[Cristian Fogel, QA [:cfogel]]

managed to reproduce the crash on macOS 10.13 with 74.0b2.
Verified and confirmed fix with 74.0b4.