1432950 - Remove the "scrollbar-base" binding

Status: RESOLVED FIXED

(Core :: XUL, task)

Description

[Brian Grinstead [:bgrins]]

+++ This bug was initially created as a clone of Bug #1431246 +++

Spinning this off as per https://bugzilla.mozilla.org/show_bug.cgi?id=1431246#c12:

There's a scrollbar-base binding that is both attached to <scrollcorner> elements and extended by the "scrollbar" binding. It attaches <handler>s for "contextmenu", "click", "dblclick", and "command" to stopPropagation (and preventDefault for "contextmenu" and "click".

However, this appears to be unnecessary given the existence of `IsEventStoppedFromAnonymousScrollbar`.

- https://dxr.mozilla.org/mozilla-central/rev/b0baaec09caf3e1b30ec6b238f5c46ef9b3188be/toolkit/content/widgets/scrollbar.xml#14-21

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

The events that get silenced here are already covered by IsEventStoppedFromAnonymousScrollbar

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Are the scrollbars used also elsewhere, outside native anonymous content?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

tree.xml and autocomplete.xml at least use <xul:scrollbar>

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

https://reviewboard.mozilla.org/r/215440/#review221088

I'd be rather surprised if this doesn't break <xul:tree> somehow.
(Feel free to explain why this is fine and ask review again.)

Comment 5

[Brian Grinstead [:bgrins]]

This doesn't remove the scrollbar binding entirely, but rather the scrollbar-base binding that it extends. So it effectively gets rid of the <handler> tags running on <xul:scrollbar> and <xul:scrollcorner>. There's more context in Bug 1431246, Comments 4-12 but AFAICT those event handlers aren't needed since nsXULElement::IsEventStoppedFromAnonymousScrollbar special cases the same events originating from those two elements.

Try appears green with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2656fed9dbfaaac297e1568b24108f3d8e4c130d.

I did some manual testing on <textarea> to make sure this didn't regress Bug 70698, and I can do more manual testing specifically on <xul:tree> if you think that could be a problem.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

IsEventStoppedFromAnonymousScrollbar is only about native anonymous content, the scrollbar in <tree> isn't native anonymous.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #6)
> IsEventStoppedFromAnonymousScrollbar is only about native anonymous content,
> the scrollbar in <tree> isn't native anonymous.

Ah I see that now - thanks. I'll look more into it, I wonder why this uses a <xul:scrollbar> directly instead of normal overflow.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Because the scrollbar in <tree> doesn't really scroll anything, but just ends up updating the view.

Comment 9

[Brian Grinstead [:bgrins]]

One extra benefit to getting rid of the handlers (in addition to one less binding) is that there won't be any more JS running on scrollbars, so we shouldn't have to create the extra global for XBL execution on pages when a user interacts with the scrollbar. I don't think we have data on it, but I have to assume scrollbars are the most likely cause of in-content XBL running JS (maybe aside from <input> / <textarea> key handling).

One maybe easy way (although I haven't tested it) to get that benefit without solving <tree> would be if we had a new binding for scrollbars in trees that included the handlers.  So instead of:

- scrollbar-base [bindToUntrustedContent]
-- scrollbar [bindToUntrustedContent]

We could make it like this:

- scrollbar [bindToUntrustedContent]
-- scrollbar-tree

Comment 10

[Bobby Holley (:bholley)]

Yeah, separating those use-cases makes a lot of sense to me. The bindings are small enough that it seems like it would be fine to also have no inheritance relationship at all, if that were to make anything easier.

Comment 11

[Brian Grinstead [:bgrins]]

One of the three places <xul:scrollbar> is used directly (autocomplete-treerows) will be removed in Bug 1427363

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

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

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: print-memory-on-data-uri.patch

Referring to Comment 9, I did some testing with this patch. Doing:

./mach run --profile $(mktemp -d) "data:text/html,<div style='height:1000px'</div>"

Will start printing out the memory used by this page on an interval. I see something like:

Total bytes: 262568
Total bytes: 262568
Total bytes: 262568
...

Then when I click on the scrollbar it goes up to:

Total bytes: 330872

Which lines up with the numbers I was seeing on a single binding in the measurements at https://bugzilla.mozilla.org/show_bug.cgi?id=1426492#c10.

With the patch applied the usage doesn't jump after clicking on the scrollbar. It doesn't save too much, but it's better than nothing.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

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

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

https://reviewboard.mozilla.org/r/215440/#review225794

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

https://reviewboard.mozilla.org/r/215440/#review226004

::: toolkit/content/widgets/tree.xml:41
(Diff revision 3)
>            <children/>
>          </xul:treerows>
>          <xul:textbox anonid="input" class="tree-input" left="0" top="0" hidden="true"/>
>        </xul:stack>
>        <xul:hbox xbl:inherits="collapsed=hidehscroll">
> -        <xul:scrollbar orient="horizontal" flex="1" increment="16" style="position:relative; z-index:2147483647;"/>
> +        <xul:scrollbar orient="horizontal" flex="1" increment="16" style="position:relative; z-index:2147483647;" oncontextmenu="event.stopPropagation();event.preventDefault();" onclick="event.stopPropagation();event.preventDefault();" ondblclick="event.stopPropagation();" oncommand="event.stopPropagation();"/>

nit: please put each on* attribute on its own line and add a space after the first semi-colon

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7080e442652d
Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content;r=dao,smaug

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7080e442652d