1510985 - Unship/Disable event.returnValue in 64

Status: RESOLVED FIXED

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

Description

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

We have to disable event.returnValue in 64 to fix regressions. The plan is to re-ship this along with window.event work in 65.

Hi Masayuki,
Can you please take care of this bug? Thank you.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e560e6bc82c8aaca2e0f9b22a2183ce8cbacf0d

Comment 2

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

Sigh... We cannot use |Pref=| in Event.webidl because of exposed to Worker. We need to completely remove returnValue from the tree.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=015e2f546fe3f4e91787a2bf8c3993abc6ec1e82

Comment 4

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

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

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=211f204779f5ae864e31cb8091be6cf00961f1ad

Comment 6

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

Attachment: bug1510985-1-beta.diff

Event.returnValue causes breaking a bank application.  The reason of the
breakage is, the web app assumes that Event.returnValue is available only
on IE and it hits same incompatibility issue with window.event (bug 1479964).
Additionally, the app may be used in various domains including in intranet.
Therefore, we cannot disable it with blacklist of domains like other similar
changes.

Fortunately, we can ship new keypress event behaviors which is
tracked by bug 1479964 in 65.  So, for backward compatibility with 62 or
earlier, we should temporarily disable Event.returnValue in 64.

But unfortunately, this was introduced in 63.  So, if some web apps have
started to use this legacy attribute, we need to contact their vendors, though.

Finally, we cannot use "Pref=" in Event.webidl because Event is exposed to
Worker and System (you'll see "[Pref] used on an interface member that is not
Window-only" error if you use it).  Therefore, we need to remove the
implementation completely because Event.returnValue should return undefined
rather than false.

(FYI: This shouldn't be landed into m-c.)

Comment 7

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

smaug:

Could you review the patch? You reject review requests from bugzilla.

Comment 8

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

Attachment: bug1510985-1-beta.diff

Well, it included change of all.js.

Comment 9

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

You can use https://searchfox.org/mozilla-central/source/dom/base/DOMPrefsInternal.h for things which are used in workers and main thread. See for example https://searchfox.org/mozilla-central/search?q=dom_netinfo_enabled&path=
StaticPrefList.h would have atomic value and then DOMPrefs creates the method on can use webidl
Func=

Comment 10

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

(In reply to Olli Pettay [:smaug] (high review load) from comment #9)
> You can use
> https://searchfox.org/mozilla-central/source/dom/base/DOMPrefsInternal.h for
> things which are used in workers and main thread. See for example
> https://searchfox.org/mozilla-central/search?q=dom_netinfo_enabled&path=
> StaticPrefList.h would have atomic value and then DOMPrefs creates the
> method on can use webidl
> Func=

With the approach, I hit this assertion:
> "Assertion failure: IsAtomic<bool>::value || NS_IsMainThread() (Non-atomic static pref 'dom.event.any.returnValue.enabled' being accessed on background thread), at m:/fx64-beta-dbg/dist/include\mozilla/StaticPrefList.h:187". The stack is here:

> #01: mozilla::dom::DefinePrefable<const JSPropertySpec> (m:\beta\dom\bindings\BindingUtils.cpp:709)
> #02: mozilla::dom::DefineProperties (m:\beta\dom\bindings\BindingUtils.cpp:988)
> #03: mozilla::dom::CreateInterfaceObjects (m:\beta\dom\bindings\BindingUtils.cpp:1068)
> #04: mozilla::dom::Event_Binding::CreateInterfaceObjects (m:\fx64-beta-dbg\dom\bindings\EventBinding.cpp:1664)
> #05: mozilla::dom::GetPerInterfaceObjectHandle (m:\beta\dom\bindings\BindingUtils.cpp:4197)
> #06: mozilla::dom::CloseEvent_Binding::CreateInterfaceObjects (m:\fx64-beta-dbg\dom\bindings\CloseEventBinding.cpp:731)
> #07: mozilla::dom::GetPerInterfaceObjectHandle (m:\beta\dom\bindings\BindingUtils.cpp:4197)
> #08: mozilla::dom::CloseEvent_Binding::GetConstructorObject (m:\fx64-beta-dbg\dom\bindings\CloseEventBinding.cpp:791)
> #09: mozilla::dom::RegisterWorkerBindings (m:\fx64-beta-dbg\dom\bindings\RegisterWorkerBindings.cpp:142)
> #10: mozilla::dom::WorkerPrivate::RegisterBindings (m:\beta\dom\workers\RegisterBindings.cpp:22)
> #11: mozilla::dom::WorkerPrivate::GetOrCreateGlobalScope (m:\beta\dom\workers\WorkerPrivate.cpp:5303)
> #12: mozilla::dom::`anonymous namespace'::ScriptExecutorRunnable::PreRun (m:\beta\dom\workers\ScriptLoader.cpp:2021)
> #13: mozilla::dom::WorkerRunnable::Run (m:\beta\dom\workers\WorkerRunnable.cpp:269)
> #14: nsThread::ProcessNextEvent (m:\beta\xpcom\threads\nsThread.cpp:1233)
> #15: NS_ProcessNextEvent (m:\beta\xpcom\threads\nsThreadUtils.cpp:530)
> #16: mozilla::dom::WorkerPrivate::RunCurrentSyncLoop (m:\beta\dom\workers\WorkerPrivate.cpp:4218)
> #17: mozilla::dom::`anonymous namespace'::LoadAllScripts (m:\beta\dom\workers\ScriptLoader.cpp:2279)
> #18: mozilla::dom::workerinternals::LoadMainScript (m:\beta\dom\workers\ScriptLoader.cpp:2401)
> #19: mozilla::dom::`anonymous namespace'::CompileScriptRunnable::WorkerRun (m:\beta\dom\workers\WorkerPrivate.cpp:377)
> #20: mozilla::dom::WorkerRunnable::Run (m:\beta\dom\workers\WorkerRunnable.cpp:380)
> #21: nsThread::ProcessNextEvent (m:\beta\xpcom\threads\nsThread.cpp:1233)
> #22: NS_ProcessNextEvent (m:\beta\xpcom\threads\nsThreadUtils.cpp:530)
> #23: mozilla::dom::WorkerPrivate::DoRunLoop (m:\beta\dom\workers\WorkerPrivate.cpp:3289)
> #24: mozilla::dom::workerinternals::`anonymous namespace'::WorkerThreadPrimaryRunnable::Run (m:\beta\dom\workers\RuntimeService.cpp:2766)
> #25: nsThread::ProcessNextEvent (m:\beta\xpcom\threads\nsThread.cpp:1233)
> #26: NS_ProcessNextEvent (m:\beta\xpcom\threads\nsThreadUtils.cpp:530)
> #27: mozilla::ipc::MessagePumpForNonMainThreads::Run (m:\beta\ipc\glue\MessagePump.cpp:334)
> #28: MessageLoop::RunHandler (m:\beta\ipc\chromium\src\base\message_loop.cc:319)
> #29: MessageLoop::Run (m:\beta\ipc\chromium\src\base\message_loop.cc:299)
> #30: nsThread::ThreadFunc (m:\beta\xpcom\threads\nsThread.cpp:507)
> #31: _PR_NativeRunThread (m:\beta\nsprpub\pr\src\threads\combined\pruthr.c:406)
> #32: pr_root (m:\beta\nsprpub\pr\src\md\windows\w95thred.c:138)
> #33: o_exp[C:\WINDOWS\System32\ucrtbase.dll +0x203ba]
> #34: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x17e94]
> #35: patched_BaseThreadInitThunk (m:\beta\mozglue\build\WindowsDllBlocklist.cpp:709)
> #36: RtlUserThreadStart[C:\WINDOWS\SYSTEM32\ntdll.dll +0x67ad1]

Comment 11

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

When I started the debug build (of course before showing the window).

Comment 12

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

Are you using atomic value for the pref?

Comment 13

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

(In reply to Olli Pettay [:smaug] (high review load) from comment #12)
> Are you using atomic value for the pref?

What means "atomic value"? I wrote your suggestion as:

DOMPrefsInternal.h:

> DOM_WEBIDL_PREF(dom_event_any_returnValue_enabled)

StaticPrefList.h:

> // If this is true, event.returnValue is available.
> VARCACHE_PREF(
>   "dom.event.any.returnValue.enabled",
>    dom_event_any_returnValue_enabled,
>   bool, false
> )

Event.webidl:

> [Constructor(DOMString type, optional EventInit eventInitDict),
>  Exposed=(Window,Worker,System), ProbablyShortLivingWrapper,
>  Func="mozilla::dom::DOMPrefs::dom_event_any_returnValue_enabled"]
> interface Event {
> ...
>   [NeedsCallerType,
>    Func="mozilla::dom::DOMPrefs::dom_event_any_returnValue_enabled"]
>   attribute boolean returnValue;

Comment 14

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

Even if I remove |Func="mozilla::dom::DOMPrefs::dom_event_any_returnValue_enabled"| from |interface Event|, I hit same assertion. I guess dom::Event is created in non-main thread first time and the pref is accessed during it.

Comment 15

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

See https://searchfox.org/mozilla-central/search?q=dom_netinfo_enabled&path

https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/modules/libpref/init/StaticPrefList.h#394-395

Comment 16

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80ac5e76c11917d72f94e08af1e6bb1212a7e0f4

Comment 17

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

Attachment: bug1510985-1-beta.diff

Thank you, looks like that this works fine (although, still running on tryserver).

I guess that I can keep waking for 1~2 hours...

Comment 18

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

Comment on attachment 9028931 [details] [diff] [review]
bug1510985-1-beta.diff

Sigh, this caused a lot of orange, and I cannot keep waking up anymore... And I cannot work on this on this weekend. Feel free to take this bug.

Comment 19

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

oh, the patch has a bug. Let me try another try push.

Comment 20

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

Attachment: bug1510985-1-beta_2.diff

This is masayuki's patch with minor change the preference name and removing accidental (I think) Func= on Event *interface*.

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

Comment 21

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

The cl failure isn't coming from this patch, so, looking good.

Hmm, but I guess I should push a beta/try too.

Comment 22

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

Attachment: bug1510985-1-beta_2_REALLY_FOR_BETA.diff

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e68d09141da7e452a178a64843f7ce638d8d6cdc
remote: recorded changegroup in replication log in 0.034s

Comment 23

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

Masayuki's patch had explanation for this all:

Event.returnValue causes breaking a bank application.  The reason of the
breakage is, the web app assumes that Event.returnValue is available only
on IE and it hits same incompatibility issue with window.event (bug 1479964).
Additionally, the app may be used in various domains including in intranet.
Therefore, we cannot disable it with blacklist of domains like other similar
changes.

Fortunately, we can ship new keypress event behaviors which is
tracked by bug 1479964 in 65.  So, for backward compatibility with 62 or
earlier, we should temporarily disable Event.returnValue in 64.

But unfortunately, this was introduced in 63.  So, if some web apps have
started to use this legacy attribute, we need to contact their vendors, though.

Comment 24

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

Comment on attachment 9029087 [details] [diff] [review]
bug1510985-1-beta_2_REALLY_FOR_BETA.diff

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1452569

User impact if declined: See comment 23


(Also asking feedback from miket, to ensure this is ok for webcompat )

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): Changing APIs is risky

String changes made/needed: NA

Comment 25

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

Comment on attachment 9029087 [details] [diff] [review]
bug1510985-1-beta_2_REALLY_FOR_BETA.diff

oh, and this is Masayuki's patch basically, r=smaug

Comment 26

[Mike Taylor [:miketaylr]]

Comment on attachment 9029087 [details] [diff] [review]
bug1510985-1-beta_2_REALLY_FOR_BETA.diff

lgtm, thanks smaug and masayuki.

Comment 27

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

Thank you for your work, smaug. And sorry for that I'm not available the weekend due to my private matter.

Comment 28

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9029087 [details] [diff] [review]
bug1510985-1-beta_2_REALLY_FOR_BETA.diff

unship event.returnValue for webcompat regression, approved for 64.0 rc1

Comment 29

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-release/rev/5856e2411504

https://hg.mozilla.org/releases/mozilla-beta/rev/71f7e3ce93e22380502eb2abcf9b1155baa4e353 (FIREFOX_64b_RELBRANCH)

Comment 30

[Julien Cristau [:jcristau] (back April 22)]

Thanks a lot Masayuki and Olli, much appreciated!

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The wpt tests need to be updated for this: https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&resultStatus=usercancel%2Crunnable%2Ctestfailed%2Cbusted%2Cexception%2Cretry&group_state=expanded&revision=321f1fb3b41f81d84adef4a7ba1fccf8f81f8364&selectedJob=215299872
At the moment they fail on release and beta's geckoview branch (other branch is similar to central with version 65).

Comment 32

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

Only partial patch landed

Comment 33

[Julien Cristau [:jcristau] (back April 22)]

backed out the wrong change and re-landed the full patch:

remote:   https://hg.mozilla.org/releases/mozilla-release/rev/c814d6d044cc37ba22670488883c17a363ce33de
remote:   https://hg.mozilla.org/releases/mozilla-release/rev/80a34446b74a1ee3d0d5e3bf69232b8b0a5e64ab

Sorry for messing this up.

Comment 34

[Julien Cristau [:jcristau] (back April 22)]

Grafted to FIREFOX_64b_RELBRANCH:
https://hg.mozilla.org/releases/mozilla-beta/rev/61f573d8a907b5b6d89cdd8b03d150096e86e103
https://hg.mozilla.org/releases/mozilla-beta/rev/cfb1a45b322e63746074f79741837063fc3651e0

Comment 35

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

Thank you Olli and Masayuki to make it happen!

Comment 36

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

Release Note Request (optional, but appreciated)
[Why is this notable]:
Event.returnValue was available since 63, but we need to disable it in 64 temporarily due to breaking some web apps. With fixing compat-issues of keypress event, it'll be available again staring from 65. So, if some web apps have started to use this legacy attribute due to available in any browsers, they would be broken (must be rare case, though).

[Affects Firefox for Android]:
Yes.

[Suggested wording]:
Event.returnValue has been shipped starting from Firefox 63, but unfortunately, it causes breaking some web apps which assume that Event.returnValue is available only on browsers which fire "keypress" events exactly same as IE.  Firefox 65 will have some improvement for the compatibility issues.  Therefore, it's disabled in 64 temporarily.

[Links (documentation, blog post, etc)]:
https://developer.mozilla.org/en-US/docs/Web/API/Event/returnValue

Comment 37

[Kohei Yoshino [:kohei]]

Updated the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/support-for-event-returnvalue-has-been-added/

Comment 38

[Julien Cristau [:jcristau] (back April 22)]

This seems too much of a corner case to call out in the main release notes, though would be great to have on https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64