1355497 - Crash in mozilla::WidgetEvent::PreventDefault

Status: RESOLVED FIXED

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

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-78985ed5-9706-45a5-a1c6-9ab672170405.
=============================================================

Seen while looking at crash stats. Crashes started using 20170404030204: http://bit.ly/2p3Q5Lt

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=38894655c89e68bcd8f45d31a0d3005f2c2b53db&tochange=b5d8b27a753725c1de41ffae2e338798f3b5cacd

Comment 1

[Mats Palmgren (inactive)]

MOZ_CRASH Reason: 	MOZ_RELEASE_ASSERT(!aCalledByDefaultHandler || mMessage != ePointerDown)

http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/widget/BasicEvents.h#456

Comment 2

[Ming-Chou Shih [:stone]]

This assert is to make sure no chrome code call peventDefault of pointerdown. Searching by searchfox but can't find the chrome code to listen to the pointerdown event. Wondering that is there anything I missed?

Comment 3

[Ming-Chou Shih [:stone]]

Wondering if this is related to some add-on.

Comment 4

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

Looks like that some add-ons have pointerdown event listener (including its libraries, e.g., jQuery). It's difficult to say what should we do here. We might be able to notify addon developers of this issue. However, they may be busy due to porting their addons to WebExtensions or just angry.

* Should we handle preventDefault() as is?
* Should we ignore calls of preventDefault() when it's called by chrome script?
* Should we raise an exception when preventDefault() is called in chrome script?

Comment 5

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] from comment #4)
> Looks like that some add-ons have pointerdown event listener (including its
> libraries, e.g., jQuery). It's difficult to say what should we do here. We
> might be able to notify addon developers of this issue. However, they may be
> busy due to porting their addons to WebExtensions or just angry.
And it's hard to identify which add-on causes it unless there are many crashes, which sounds not good.

> * Should we handle preventDefault() as is?
> * Should we ignore calls of preventDefault() when it's called by chrome
> script?
Perhaps we could ignore calls by chrome script to prevent the content behaviors impacted by incorrect chrome code?

> * Should we raise an exception when preventDefault() is called in chrome
> script?
Sounds good to have some warnings/notifications to let users know what's going on. But have no idea how to change the webidl since the original API doesn't throw an exception?

Comment 6

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

smaug:

Do you think that making preventDefault() raise exception is okay?

Comment 7

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

raising exception sounds odd.

Could we perhaps create a warning showing up in web or browser console, but otherwise just normally handle the call as is.
And for FF internal JS code the assertion could be MOZ_ASSERT still to catch issues in debug builds.


I wonder about WebExtensions. How do such add event listeners and where and do they use system group or do they show up in event handling as chrome JS.

Comment 8

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

If WebExtensions behave as normal web pages, then this issue would go away when addons go away and there are only WebExtensions. But I don't know about the setup WebExtensions have.

Comment 9

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

(In reply to Olli Pettay [:smaug] from comment #8)
> If WebExtensions behave as normal web pages, then this issue would go away
> when addons go away and there are only WebExtensions. But I don't know about
> the setup WebExtensions have.

Addons created with WebExtensions can have both script. When it needs to catch events or touch DOM tree in content, they insert script into the content. This is behave same as content script.  If they need to use API, e.g., download, they can insert script into chrome too. In my understanding, this is chrome script.  However, this is NOT in content. So, preventing pointer down might be safe (if it's not dispatched in chrome DOM tree when it's fired in content).  The former might cause problem. If it catches pointerdown event before web contents, e.g., window.addEventListener("pointerdown", func, true), it can prevent following mousedown event. But I don't know how we can distinguish if it's in WebExtension's script or content's script.

Comment 10

[Ming-Chou Shih [:stone]]

(In reply to Masayuki Nakano [:masayuki] from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > If WebExtensions behave as normal web pages, then this issue would go away
> > when addons go away and there are only WebExtensions. But I don't know about
> > the setup WebExtensions have.
> 
> Addons created with WebExtensions can have both script. When it needs to
> catch events or touch DOM tree in content, they insert script into the
> content. This is behave same as content script.  If they need to use API,
> e.g., download, they can insert script into chrome too. In my understanding,
> this is chrome script.  However, this is NOT in content. So, preventing
> pointer down might be safe (if it's not dispatched in chrome DOM tree when
> it's fired in content).  The former might cause problem. If it catches
> pointerdown event before web contents, e.g.,
> window.addEventListener("pointerdown", func, true), it can prevent following
> mousedown event.
I had thought it's ok to let content script to preventDefault on pointerdown. May I learn more about your concerns?

Comment 11

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

(In reply to Masayuki Nakano [:masayuki] from comment #9)
> e.g., download, they can insert script into chrome too. In my understanding,
> this is chrome script. 
Is it? Or does it use non-system-principal?

Comment 12

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

(In reply to Ming-Chou Shih [:stone] from comment #10)
> (In reply to Masayuki Nakano [:masayuki] from comment #9)
> > (In reply to Olli Pettay [:smaug] from comment #8)
> > > If WebExtensions behave as normal web pages, then this issue would go away
> > > when addons go away and there are only WebExtensions. But I don't know about
> > > the setup WebExtensions have.
> > 
> > Addons created with WebExtensions can have both script. When it needs to
> > catch events or touch DOM tree in content, they insert script into the
> > content. This is behave same as content script.  If they need to use API,
> > e.g., download, they can insert script into chrome too. In my understanding,
> > this is chrome script.  However, this is NOT in content. So, preventing
> > pointer down might be safe (if it's not dispatched in chrome DOM tree when
> > it's fired in content).  The former might cause problem. If it catches
> > pointerdown event before web contents, e.g.,
> > window.addEventListener("pointerdown", func, true), it can prevent following
> > mousedown event.
> I had thought it's ok to let content script to preventDefault on
> pointerdown. May I learn more about your concerns?

If content script of add-on prevents the default of "pointerdown", mousedown and click events won't be fired. That may be convenient for developers who intentionally want to prevent compatibility events. Otherwise, like some add-ons using jQuery, they may cause trouble in a lot of websites.

(In reply to Olli Pettay [:smaug] from comment #11)
> (In reply to Masayuki Nakano [:masayuki] from comment #9)
> > e.g., download, they can insert script into chrome too. In my understanding,
> > this is chrome script. 
> Is it? Or does it use non-system-principal?

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/background
Ah, I may be confused. I think that background script cannot use XPCOM interface. So, they may run in non-system-principal. They might be just allowed to use APIs of WebExtensions (additionally, it depends on manifest).

Comment 13

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

So, I worry about like this case if addons can prevent default of "pointerdown".

If addon does this in content script:

> foo.addEventListener("pointerdown", handler);
> foo.addEventListener("mousedown", handler);
> 
> function handler(event)
> {
>   // Handle the event here.
>   event.preventDefault();
> }

web content can receive mousedown at capturing phase normally if it's on PointerEvents non-supported browser. And also web content can receive default prevented mousedown event at bubbling phase too.

However, if PointerEvents is supported, web content cannot receive "mousedown" anymore.

It might be necessary to add new manifest for WebExtentions which can use PointerEvent. If there is a special permission for it, we might be able to check it like https://reviewboard.mozilla.org/r/125656/diff/3#index_header

Comment 14

[Jim Mathies [:jimm]]

Hey Masayuki, can you suggest next steps on this? It's low volume, wondering if it needs to track 55 as well.

Comment 15

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

(In reply to Jim Mathies [:jimm] from comment #14)
> Hey Masayuki, can you suggest next steps on this? It's low volume, wondering
> if it needs to track 55 as well.

I think that this only occurs only when PointerEvents is enabled. So, this should be just one of the blockers.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Talked with Stone, after discussion with Masayuki, he got an idea of how to proceed this. He will be back to this after critical quantum task on his plate.

Also, fix-optional looks the right flag, as Pointer events has been enabled on only Windows Nightly and the crash volume is quite low.

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Add permission 'pointerEvent' for WebExtensions to prevent default on pointerdown to stop firing mouse events

Comment 18

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

Comment on attachment 8890147 [details] [diff] [review]
Add permission 'pointerEvent' for WebExtensions to prevent default on pointerdown to stop firing mouse events

># HG changeset patch
># User Stone Shih <[e-mail]>
># Date 1499676121 -28800
>#      Mon Jul 10 16:42:01 2017 +0800
># Node ID 8e56f5817a34a3d7ed71a3c0e2b571608894fd93
># Parent  0faada5c2f308f101ab7c54a87b3dce80b97d0e3
>Bug 1355497: Add permission 'pointerEvent' for WebExtensions to prevent default on pointerdown to stop firing mouse events

Please explain the reason why we need to do this change.

>diff --git a/toolkit/components/extensions/schemas/manifest.json b/toolkit/components/extensions/schemas/manifest.json
>--- a/toolkit/components/extensions/schemas/manifest.json
>+++ b/toolkit/components/extensions/schemas/manifest.json
>@@ -240,17 +240,18 @@
>         "choices": [
>           {
>             "type": "string",
>             "enum": [
>               "clipboardRead",
>               "clipboardWrite",
>               "geolocation",
>               "idle",
>-              "notifications"
>+              "notifications",
>+              "pointerEvent"

How about "preventDefaultOfPointerDown"? Because this doesn't restrict other pointer events behavior.

>+void
>+WidgetEvent::PreventDefault(bool aCalledByDefaultHandler)
>+{
>+  if (mMessage == ePointerDown) {
>+    if (aCalledByDefaultHandler) {
>+      // Shouldn't prevent default on pointerdown by defualt handlers to stop
>+      // firing legacy mouse events. Use MOZ_ASSERT to catch incorrect usages
>+      // in debug builds
>+      MOZ_ASSERT(false);
>+      return;
>+    } else {

You don't need this else block since above if case always reaches "return". So, reduce following indent level.

>+      nsIPrincipal* principal = nsContentUtils::SubjectPrincipal();
>+      nsAutoString addonId;
>+      NS_ENSURE_SUCCESS_VOID(principal->GetAddonId(addonId));

Please do not use NS_ENSURE_* in new code. Please use |if (NS_WARN_IF(...)) {| style.

>+      if (!addonId.IsEmpty() &&
>+          !BasePrincipal::Cast(principal)->AddonHasPermission(NS_LITERAL_STRING("pointerEvent"))) {

nit: too long line. How about the following style?

>      if (!addonId.IsEmpty() &&
>          !BasePrincipal::Cast(principal)->
>            AddonHasPermission(
>              NS_LITERAL_STRING("preventDefaultOfPointerDown"))) {

>+        // Ignore the case that it's called by a web extension w/o permission.
>+        return;
>+      }
>+      nsContentUtils::ReportToConsoleNonLocalized(
>+        NS_LITERAL_STRING("Prevent default on pointerdown will stop firing legacy mouse events"),
>+        nsIScriptError::warningFlag,
>+        NS_LITERAL_CSTRING("Pointer Event"),
>+        nullptr);

If pointerdown event handler in content calls Event.preventDefault(), we're here. So, if it's not in addon's context, it's odd to warn like this.

BTW, shouldn't this be a localizable message? I don't find the guideline about whether it's allowed to use non-localized message or not in this case. However, nsContentUtils::ReportToConsoleNonLocalized() isn't used in so many places.

>+    }
>+  }
>+  mFlags.PreventDefault(aCalledByDefaultHandler);
>+}


Anyway, I suggested to use permission. So, the approach should be check by smaug too. And I wonder, does somebody manage the permissions? If so, this patch needs such person's review too.

Comment 19

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

(FWIW, I prefer NS_ENSURE_* ;) Some prefer NS_WARN_IF, some NS_ENSURE_* and there isn't really any rule which one to use.)

I wonder if we really need the permission. Could we first try living without it and if someone asks for the behavior, then add it?

Comment 20

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

Comment on attachment 8890147 [details] [diff] [review]
Add permission 'pointerEvent' for WebExtensions to prevent default on pointerdown to stop firing mouse events

I would really prefer try without some magical permission first.
Or if you want permission, ask WebExtension folks

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #19)
> (FWIW, I prefer NS_ENSURE_* ;) Some prefer NS_WARN_IF, some NS_ENSURE_* and
> there isn't really any rule which one to use.)

See here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros
NS_ENSURE_* isn't invalid but looks like that NS_WARN_IF is preferred.

> I wonder if we really need the permission. Could we first try living without
> it and if someone asks for the behavior, then add it?

But some addons might kill legacy mouse events without understanding what they do. Is it okay?

Comment 22

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

I mean we'd default to the same handling as if addon wouldn't have the permission.

Comment 23

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

(In reply to Olli Pettay [:smaug] from comment #22)
> I mean we'd default to the same handling as if addon wouldn't have the
> permission.

Ah, sounds reasonable.

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Stone, sorry I failed to recall our last chat about this bug. What's the next step here? Is it still viewed as a blocker for shipping Pointer Events post 57? thank you.

Comment 25

[Ming-Chou Shih [:stone]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> Stone, sorry I failed to recall our last chat about this bug. What's the
> next step here?
I'm revising my patch.

> Is it still viewed as a blocker for shipping Pointer Events
> post 57?
I think it is.

Comment 26

[Ming-Chou Shih [:stone]]

Attachment: Ignore preventDefault on pointerdown by WebExtensions

Comment 27

[Ming-Chou Shih [:stone]]

Comment on attachment 8910673 [details] [diff] [review]
Ignore preventDefault on pointerdown by WebExtensions

This patch includes
1. Assert if a default handler calls preventDefault on pointerdown.
2. Ignore if a web extension calls preventDefault on pointerdown.
3. Otherwise, do preventDefault
4. Add a web extension test case to make sure preventDefault is bypassed.

Comment 28

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

Comment on attachment 8910673 [details] [diff] [review]
Ignore preventDefault on pointerdown by WebExtensions

>+WidgetEvent::PreventDefault(bool aCalledByDefaultHandler)
>+{
>+  if (mMessage == ePointerDown) {
>+    if (aCalledByDefaultHandler) {
>+      // Shouldn't prevent default on pointerdown by a defualt handlers to stop
default

>+      // firing legacy mouse events. Use MOZ_ASSERT to catch incorrect usages
>+      // in debug builds
>+      MOZ_ASSERT(false);
>+      return;
>+    }
>+    nsIPrincipal* principal = nsContentUtils::SubjectPrincipal();
I'm not too happy with SubjectPrincipal call here.
If there isn't any JS on stack, this will crash. Can we please pass principal explicitly
from Event::PreventDefault(JSContext* aCx, CallerType aCallerType) when needed, and if
principal isn't passed, we treat this as if it wasn't an addon.

nsContentUtils::SubjectPrincipal(JSContext* aCx) might be useful.

Comment 29

[Ming-Chou Shih [:stone]]

Attachment: Ignore preventDefault on pointerdown by WebExtensions

Comment 30

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

Comment on attachment 8911753 [details] [diff] [review]
Ignore preventDefault on pointerdown by WebExtensions

Don't you need to enable pointer events in the test? Otherwise the test will fail when run beta or so.

Comment 31

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fb682eb87cce0706b0f5ca4132a780e9f7e4988

Comment 32

[Ming-Chou Shih [:stone]]

(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8911753 [details] [diff] [review]
> Ignore preventDefault on pointerdown by WebExtensions
> 
> Don't you need to enable pointer events in the test? Otherwise the test will
> fail when run beta or so.

Fixed.

Comment 33

[Ming-Chou Shih [:stone]]

Attachment: Ignore preventDefault on pointerdown by WebExtensions

Add the handling for the case that calls preventDefault on other threads. Please help me to take a look if it's ok. Thanks.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d58c76e7481
Ignore preventDefault on pointerdown by WebExtensions. r=smaug.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d58c76e7481