1484186 - PointerEvent.preventDefault() does nothing in WebExtensions, but works in content scripts

Status: ASSIGNED

(Core :: DOM: Events, defect, P3)

Description

[CoolCmd]

Attachment: test case

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180807170231

Steps to reproduce:

1.load attached extension
2.new tab will open
3.open DevTools console
4.click anywhere on page


Actual results:

you will see the message in console:
After preventDefault() type=pointerdown defaultPrevented=false

preventDefault() does nothing: selection, drag&drop, etc works.



Expected results:

you should see the message in console:
After preventDefault() type=pointerdown defaultPrevented=true

selection, drag&drop, etc do NOT work.

if you load coolcmd.html outside the extension (file://, http://) then you should see this behavior.

Comment 1

[Masatoshi Kimura [:emk]]

This behavior is intentionally introduced (see bug 1355497). But in this case, coolcmd.html is an extension page (moz-extension://...) created by chrome.tabs.create().

Is it intended to block preventDefault() even on extension's own pages?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, it's intentional behavior as Kimura-san said.

If addons prevents default of pointerdown, it stops dispatching mosuedown/mouseup/click/dblclick events into web content. This causes INVALID bug reports and makes us investigate the cause harder.  Therefore, we do not allow any chome code use preventDefault() of pointerdown.

On the other hand, as Kimura-san said, it seems that it's better to allow using prentDefault() of pointerdown in addon-secific page. However, I have no idea how to distinguish whether it's called in addon-specific page especially when WidgetEvent::PreventDefault() is called.

Smaug, do you have idea?

Comment 3

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Would be odd if addon could preventDefault on some pages but not on others, IMO.

Anyhow, the page should have a principal on which GetIsAddonOrExpandedAddonPrincipal() returns true.

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #2)
> However, I have
> no idea how to distinguish whether it's called in addon-specific page
> especially when WidgetEvent::PreventDefault() is called.

BasePrincipal::Cast(aPrincipal)->AddonPolicy() vs. BasePrincipal::Cast(aPrincipal)->ContentScriptAddonPolicy()?

Comment 5

[Masatoshi Kimura [:emk]]

By the way, GetAddonId() returns an empty string if it is called from content scripts. So WidgetEvent::PreventDefault() behaves contrary to what was intended (blocks preventDefault() on addon pages, but does NOT block on content scripts in web pages). Obviously this is a bug.

Comment 6

[Masatoshi Kimura [:emk]]

Is this check really needed at all?
* Chrome does not block preventDefault().
* "This causes INVALID bug reports" did not happen even though we (accidentally) did not block preventDefault() for a long time.
* If this check is so important, why didn't you test the patch?

I suggest removing this useless (even harmful) check completely.

Comment 7

[CoolCmd]

:masayuki, i do not know that the phrase "" means, but content script and web page script share the DOM tree and DOM events, so that content script can communicate with web page or prevent web page from receiving arbitrary (not only input) events without ugly hacks like injecting <script> manually.

Comment 8

[CoolCmd]

* the phrase "This causes INVALID bug reports" means

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> By the way, GetAddonId() returns an empty string if it is called from
> content scripts. So WidgetEvent::PreventDefault() behaves contrary to what
> was intended (blocks preventDefault() on addon pages, but does NOT block on
> content scripts in web pages). Obviously this is a bug.

Sounds too bad...

(In reply to Masatoshi Kimura [:emk] from comment #6)
> Is this check really needed at all?
> * Chrome does not block preventDefault().

If any addons were shared with Chrome, and also Chrome users may have become victims, yes, we could remove the hack. However, I'm not sure it is the time to do it.

> * "This causes INVALID bug reports" did not happen even though we
> (accidentally) did not block preventDefault() for a long time.

Good point. On the other hand, pointer events is currently used a lot? I guess a lot of addons still listen to legacy events.

> * If this check is so important, why didn't you test the patch?

I'd be happy if I'd have much time everyday...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to CoolCmd from comment #7)
> :masayuki, i do not know that the phrase "" means, but content script and
> web page script share the DOM tree and DOM events, so that content script
> can communicate with web page or prevent web page from receiving arbitrary
> (not only input) events without ugly hacks like injecting <script> manually.

See bug 1355497. If addons call preventDefault() of pointerdown and web content listens to legacy mouse button events, mouse events won't be dispatched in the content. So, any listeners of legacy mouse button events of web apps may be killed by addons accidentally (although according to Kimura-san's investigation, the block does not work well).

Comment 11

[CoolCmd]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> dispatched in the content. So, any listeners of legacy mouse button events
> of web apps may be killed by addons accidentally (although according to
> Kimura-san's investigation, the block does not work well).
there are thousands of things that content script can "do accidentally". but trying to prevent this is not Firefox job, at least by blocking legal and useful APIs. BTW web page also can "accidentally kill" mouse events.

> However, I'm not sure it is the time to do it.
if this patch is doing only harmful things, then it should have been fixed 11 months ago.

Comment 12

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #9)
> If any addons were shared with Chrome, and also Chrome users may have become
> victims, yes, we could remove the hack. However, I'm not sure it is the time
> to do it.

If the check is currently working, we can argue whether it should be removed, yes. But there is no point in keeping the broken check and pretending to support blocking. Even if we will add a proper check later, the current check should be removed now because it is actively making a harmful effect on add-on pages.

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: Remove broken addon check from WidgetEvent::PreventDefault()

I'm reluctant to change the behavior in content scripts because some addons may already depend on the current behavior even though it is accidental. So I added telemetry to measure the current usage.

I changed GetAddonId() to AddonPolicy() because the latter is much faster (no virtual calls, memory allocation, string copy, etc.).

I also added a unit test to make sure that the current behavior is not changed accidentally.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9003981 [details] [diff] [review]
Remove broken addon check from WidgetEvent::PreventDefault()

Review of attachment 9003981 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/xpcshell/test_ext_pointerdown_preventdefault.js
@@ +19,5 @@
> +    },
> +
> +    files: {
> +      "content_script.js": async function contentScript() {
> +        let e = new PointerEvent("pointerdown", {cancelable: true});

Hmm, this test checks whether it is fine only when the event is created by JS, rather than using synthesized events. So, looks like that |e.isTrusted| is false in this case unless the content_script.js is loaded as chrome code. Unfortunately, event handling code may use different path to handle trusted events. I guess that we cannot use nsIDOMWindowUtils here since this is XPCShell test, so, it's okay to keep current code. However, if it were possible, you should add a test to check synthesized events.

@@ +21,5 @@
> +    files: {
> +      "content_script.js": async function contentScript() {
> +        let e = new PointerEvent("pointerdown", {cancelable: true});
> +        browser.test.assertFalse(e.defaultPrevented, "defaultPrevented should be initialized with false");
> +        e.preventDefault();

Could you use more similar path to check the event? That is, listening to the event at capturing phase of documentElement, and dispatch the event into an element in file_sample.html with EventTarget.dispatchEvent(). The reason of this is, BaseEventFlags manages dispatching state and some other hacky flags. So, we'd check such flags in the future (or might have already done it).

@@ +48,5 @@
> +      `,
> +      "addon_script.js": async function addonScript() {
> +        let e = new PointerEvent("pointerdown", {cancelable: true});
> +        browser.test.assertFalse(e.defaultPrevented, "defaultPrevented should be initialized with false");
> +        e.preventDefault();

Ditto.

::: toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
@@ +68,5 @@
>  skip-if = true # This test no longer tests what it is meant to test.
>  [test_ext_permission_xhr.js]
>  [test_ext_persistent_events.js]
> +[test_ext_pointerdown_preventdefault.js]
> +skip-if = os == "android"

Could you add comment to explain why this skips on Android.

::: toolkit/components/telemetry/Histograms.json
@@ +13986,5 @@
> +  "POINTERDOWN_PREVENTDEFAULT": {
> +    "record_in_processes": ["main", "content"],
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1484186],
> +    "expires_in_version": "68",

Should we stop checking this?

@@ +13990,5 @@
> +    "expires_in_version": "68",
> +    "kind": "enumerated",
> +    "n_values": 4,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Reports when preventDefault() is called for pointerdown events (0=page, 1=addon, 2=content script)."

Do we need to collect the case 0?

::: widget/WidgetEventImpl.cpp
@@ +572,5 @@
>        }
> +      if (princial->ContentScriptAddonPolicy()) {
> +        source |= 2;
> +      }
> +      Telemetry::Accumulate(Telemetry::POINTERDOWN_PREVENTDEFAULT, source);

I think that phase information may be useful. You can get it with mFlags.

1. target phase: call mFlags.InTargetPhase()
2. bubbling phase: check if mFlags.mInBubblingPhase is true
3. capturing phase: check if mFlags.mInCapturingPhase is true
4. not being dispatched: otherwise

And also, I'm not sure if this is useful telemetry probe because this counts number of preventDefault() calls of pointerdown. Isn't it what we need is, number of it per page or addon? I guess per addon may be useful and easy to implement it. For example, when we ask reporters, we'd be able to say, "can you use on of these addons?". If yes, this should be a scalar, whose key is addon ID (allowed equals or less than 72 characters) and value is bool? See  <https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/toolkit/components/telemetry/Telemetry.h#567-574>. This is used to collect the IME usage (bug 1215818). Perhaps, adding the additional information should be included in head of the key.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9003981 [details] [diff] [review]
Remove broken addon check from WidgetEvent::PreventDefault()

r- until coming new patch.