2023741 - "TypeError: node.closest is not a function" in resource://gre/actors/AutoScrollChild.sys.mjs:73

Status: NEW

(Core :: Panning and Zooming, defect, P3)

Description

[Rob Wu [:robwu]]

The log of bug 2023730 includes an error message highlighting a bug in AutoScrollChild.sys.mjs:

• "TypeError: node.closest is not a function" thrown from https://searchfox.org/firefox-main/rev/af3c77e22f88ca06e1b4ac822c1d056f263a64fe/toolkit/actors/AutoScrollChild.sys.mjs#73
• With node = event.originalTarget (where event is for a mousedown event)
• there is logic across the function that assumes node to be an element, but the error shows that it is not an element.
  • I suspect that it may be a document, which I observed before in bug 1478596.

The logic that adds the node.closest calls was introduced in bug 1716883.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

CCing Gijs who is the author of the patch for bug 1716883.

Comment 2

[:Gijs (he/him)]

Are there steps to reproduce outside of bug 2023730?

Put differently, can a human actually cause a trusted mousedown event to fire with a document node as the originalTarget?

It's easy to change https://searchfox.org/firefox-main/rev/4d26a0d0b80b56b95f9f7958a6d16dd4dfb35c1a/toolkit/actors/AutoScrollChild.sys.mjs#35 to bail out for a document node (or anything other than an element node, though that feels scarier), but there is other code in this actor (and many others) that assumes this kind of thing can't happen. Adding checks all over the place (which at best are just going to swallow errors that would cause test failures anyway!), as well as teaching all our engineers to remember to add more of those checks when they write new code, feels like a poor solution to this problem, especially if it only happens because of bad test automation / tests.

Comment 3

[Rob Wu [:robwu]]

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

Are there steps to reproduce outside of bug 2023730?

I don't know. I don't even know how to reproduce consistently; I reported this bug upon seeing the error in bug 2023730 (which itself is a TV failure).

I also linked bug 1478596 because there too we observed the target being a document, somehow. I suspect that the two bugs have related causes, the exact nature unknown to me.

It's easy to change https://searchfox.org/firefox-main/rev/4d26a0d0b80b56b95f9f7958a6d16dd4dfb35c1a/toolkit/actors/AutoScrollChild.sys.mjs#35 to bail out for a document node (or anything other than an element node, though that feels scarier), but there is other code in this actor (and many others) that assumes this kind of thing can't happen.

Line 45 does a nodeType check, which already suggests that the target can be something other than an element. I haven't looked closely at what it could/should be, but that check and the unconditional use of node.closest (line 71 and 73) seem inconsistent to me.

Overall this is not a high priority bug, I filed it in the hope of raising awareness of this issue, so we can get back to it and fix/close it once we know why the target is not an element.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Setting P3:S3 based on

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

Overall this is not a high priority bug

this comment.